Packaging jj for openSUSE: gpgsm tests failing with 0.28.x

[kastl-ars]

Dear jj maintainers,

I am packaging jj for openSUSE and using it for my daily work.

Starting with 0.28.0 the tests start failing.

Any idea how to fix these?

Kind Regards,
Johannes

1. Error

[  768s] failures:
[  768s] 
[  768s] ---- test_git_push::test_git_push_multiple::use_git2_for_remote_calls stdout ----
[  768s] 
[  768s] thread 'test_git_push::test_git_push_multiple::use_git2_for_remote_calls' panicked at /home/abuild/rpmbuild/BUILD/jujutsu-0.28.0-build/jujutsu-0.28.0/vendor/insta-1.42.2/src/runtime.rs:694:9:
[  768s] Insta does not allow inline snapshot assertions in loops. Wrap your assertions in allow_duplicates! to change this.
[  768s] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[  768s] 
[  768s] 
[  768s] failures:
[  768s]     test_git_push::test_git_push_multiple::use_git2_for_remote_calls
[  768s] 
[  768s] test result: FAILED. 843 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 151.62s
[  768s] 
[  768s] error: test failed, to rerun pass `-p jj-cli --test runner`

2nd Error

[  776s] failures:
[  776s] 
[  776s] ---- test_gpg::gpgsm_signing_roundtrip stdout ----
[  776s] 
[  776s] thread 'test_gpg::gpgsm_signing_roundtrip' panicked at lib/tests/test_gpg.rs:337:50:
[  776s] called `Result::unwrap()` on an `Err` value: InvalidSignatureFormat
[  776s] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[  776s] 
[  776s] ---- test_gpg::gpgsm_signing_roundtrip_explicit_key stdout ----
[  776s] 
[  776s] thread 'test_gpg::gpgsm_signing_roundtrip_explicit_key' panicked at lib/tests/test_gpg.rs:364:61:
[  776s] called `Result::unwrap()` on an `Err` value: InvalidSignatureFormat
[  776s] 
[  776s] ---- test_gpg::gpgsm_unknown_key stdout ----
[  776s] 
[  776s] thread 'test_gpg::gpgsm_unknown_key' panicked at lib/tests/test_gpg.rs:413:70:
[  776s] called `Result::unwrap()` on an `Err` value: InvalidSignatureFormat
[  776s] 
[  776s] 
[  776s] failures:
[  776s]     test_gpg::gpgsm_signing_roundtrip
[  776s]     test_gpg::gpgsm_signing_roundtrip_explicit_key
[  776s]     test_gpg::gpgsm_unknown_key
[  776s] 
[  776s] test result: FAILED. 448 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 7.98s
[  776s] 
[  776s] error: test failed, to rerun pass `-p jj-lib --test runner`

Summary

[  783s] error: 2 targets failed:
[  783s]     `-p jj-cli --test runner`
[  783s]     `-p jj-lib --test runner`

Full build log ist here:
https://build.opensuse.org/build/home:ojkastl_buildservice:Branch_devel_tools_scm/openSUSE_Tumbleweed/x86_64/jujutsu/_log

The tests are being called like this:

%check
rm -rf tests/contest/
%{cargo_test}

(The macro is defined here: https://github.com/openSUSE-Rust/cargo-packaging/blob/main/macros.cargo#L20)

[emilazy]

0.28.0 shouldn’t be packaged, as there were a few issues with it, the first of these test problems included, and a vulnerability fix coming in just after the release. I believe 0.28.1 should be out within the next few days.

I’m not sure about the gpgsm one. Are you giving it a gpgsm build dependency to test against?

[kastl-ars]

Thanks for the quick reply.

0.28.0 shouldn’t be packaged, as there were a few issues with it, the first of these test problems included, and a vulnerability fix coming in just after the release. I believe 0.28.1 should be out within the next few days.

I was checking the releases, and 0.28.0 is a) a proper release (and not a tag only) and b) not marked as pre-release. Normally (i.e. in other projects) this means it is good to go.

Maybe it should be marked as a pre-release or yanked, if it has issues?

I’m not sure about the gpgsm one. Are you giving it a gpgsm build dependency to test against?

gnupg is installed as dependency, until now that was enough. I have added gpgme now, too.

[emilazy]

There’s a warning about issues with the release and a new minor version being due in the release description, but it might not be loud enough. We only found some of the issues after the release was already cut, unfortunately. I suppose we could yank the jj-lib version from crates.io, seeing as we couldn’t publish the jj-cli crate.

[martinvonz]

There’s a warning about issues with the release and a new minor version being due in the release description, but it might not be loud enough. We only found some of the issues after the release was already cut, unfortunately. I suppose we could yank the jj-lib version from crates.io, seeing as we couldn’t publish the jj-cli crate.

Makes sense. Done.

[ilyagr]

0.28 is not dangerous in any way (well, no more than previous releases); the main problem with it is that it might be excessively difficult and a waste of time to package it. Sorry about that!

To expand on the story: there was some bad luck. Our CI didn't catch the issues with tests, and there was a cargo packaging issue that the CI also didn't catch. Also, (this is perhaps good luck?) the fix to GHSA-2frx-2596-x5r6 became available right after our release.

Hopefully, 0.28.1 will be released shortly, making all of this moot.

[emilazy]

We should consider doing dry-run installs and publishes in CI, I think. It'd be cheap. Not sure how the latter would work with multiple crates.

[ilyagr]

We should consider doing dry-run installs and publishes in CI, I think. It'd be cheap. Not sure how the latter would work with multiple crates.

This would require raising the version of jj-cli, jj-lib, etc inside Cargo.toml right after a release. I think this could be nice, but I remember Martin was concerned that this adds busywork and brittleness to the release. I don't currently have a clear sense of how painful that would be or how to minimize the burden.

[martinvonz]

It would be nice if we can do the test-publishing to a local crate registry that's set up just for the duration of the test. I don't know how involved that is.

[arxanas]

This would require raising the version of jj-cli, jj-lib, etc inside Cargo.toml right after a release

IIRC the most recent release issue could actually be caught without bumping the version number, and just checking cargo publish --dry-run for each crate.

[johanneskastl]

Just a quick note, 0.28.1 seems to build on at least some systems. openSUSE Leap 15.x and 16.0 are building fine, while Tumbleweed builds fail for x86_64/aarch64/etc. with three gpg-related errors:

[  722s] test test_gpg::gpgsm_signing_roundtrip ... FAILED
[  722s] test test_gpg::gpgsm_signing_roundtrip_explicit_key ... FAILED
[  722s] test test_gpg::gpgsm_unknown_key ... FAILED
[...]
[  724s] 
[  724s] failures:
[  724s] 
[  724s] ---- test_gpg::gpgsm_signing_roundtrip stdout ----
[  724s] 
[  724s] thread 'test_gpg::gpgsm_signing_roundtrip' panicked at lib/tests/test_gpg.rs:337:50:
[  724s] called `Result::unwrap()` on an `Err` value: InvalidSignatureFormat
[  724s] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[  724s] 
[  724s] ---- test_gpg::gpgsm_signing_roundtrip_explicit_key stdout ----
[  724s] 
[  724s] thread 'test_gpg::gpgsm_signing_roundtrip_explicit_key' panicked at lib/tests/test_gpg.rs:364:61:
[  724s] called `Result::unwrap()` on an `Err` value: InvalidSignatureFormat
[  724s] 
[  724s] ---- test_gpg::gpgsm_unknown_key stdout ----
[  724s] 
[  724s] thread 'test_gpg::gpgsm_unknown_key' panicked at lib/tests/test_gpg.rs:413:70:
[  724s] called `Result::unwrap()` on an `Err` value: InvalidSignatureFormat
[  724s] 
[  724s] 
[  724s] failures:
[  724s]     test_gpg::gpgsm_signing_roundtrip
[  724s]     test_gpg::gpgsm_signing_roundtrip_explicit_key
[  724s]     test_gpg::gpgsm_unknown_key
[  724s] 
[  724s] test result: FAILED. 448 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 4.95s

(Will read the rest of the comments later, just wanted to report back)

[johanneskastl]

There’s a warning about issues with the release and a new minor version being due in the release description, but it might not be loud enough.

Just my 2 cents as a distribution packager:

Releases with issues should be yanked or at least marked as pre-release. This way they are not shown as "latest" and on the repo's overview page.

I am using RSS feeds to stay up to date with releases, other packagers use Github's notification mails. Both do not send out updates for releases that get "changed" (description, etc.). So it should be as obvious as possible to find out issues with a release.

Have a nice weekend, everyone!

Kind Regards,
Johannes

[arxanas]

Releases with issues should be yanked or at least marked as pre-release. This way they are not shown as "latest" and on the repo's overview page.

Hi johanneskastl, are you proposing a specific action here, or just remarking for future reference?

On https://crates.io:

• v0.28.0 should already be yanked.
• v0.28.1 should be published.

On GitHub:

• v0.28.0 isn't marked as a "pre-release" or "latest". It seems to have built and attached some binaries fine, but it does have a note at the top remarking on the next patch version. Are you suggesting that we should change it to be a "pre-release"?
• v0.28.1 should be marked as the "latest" release.

(Unfortunately, I don't have any suggestions regarding the GPG tests. I've seen packagers in certain other situations simply mark tests as "skipped" when they weren't compatible with the environment, for various reasons.)

[ilyagr]

I can think of a couple of things to consider, though they'd require effort and experimentation.

One problem with our release workflow is such that we have to create a non-draft release for the CI to start building the binaries.

I'm not 100% sure, but we might be able to mark that release as pre-release, and the CI might still work. We could change it to a full release after the push to crates.io succeeds and after the binaries all build successfully. Then, if something goes wrong, it could stay a pre-release forever. I think we should experiment with this.

If we changed things so that we could trigger the build in some other way, the CI could create a draft release after building the binaries. Then, we could keep it invisible to our users until we're sure everything went OK.

[emilazy]

One problem with our release workflow is such that we have to create a non-draft release for the CI to start building the binaries.

I'm not 100% sure, but we might be able to mark that release as pre-release, and the CI might still work. We could change it to a full release after the push to crates.io succeeds and after the binaries all build successfully. Then, if something goes wrong, it could stay a pre-release forever. I think we should experiment with this.

If we changed things so that we could trigger the build in some other way, the CI could create a draft release after building the binaries. Then, we could keep it invisible to our users until we're sure everything went OK.

I was thinking about putting some time into more fully automating the release workflow, such that cutting a release would just be merging a release PR with no command‐running, and the merge queue would ensure the release is only cut after everything succeeds properly. That would solve this, remove unnecessary manual toil from the release instructions, and also allow us to reduce the attack surface of the crates.io owners (because GitHub Actions would be the only one ever publishing the crate). Is there interest?

(This would also make it easier for non‐maintainer contributors to contribute to the release process, which might have helped with the 0.28.x situation where we’re having to cut two patch releases.)

[martinvonz]

Sounds great if we can automate more of the releases. Thanks, @emilazy!

[ilyagr]

also allow us to reduce the attack surface of the crates.io owners (because GitHub Actions would be the only one ever publishing the crate).

I had some mild worries about this part; I think there's something to be said for keeping some secrets out of CI considering the amount of trouble it takes to keep the GitHub Actions secure. But it's a tradeoff; the CI already does generate the release binary and can edit the GitHub Releases, which are also quite security-sensitive.

In any case, automating more of the release workflow would be nice.

[thoughtpolice]

See #2483 for details on automating the whole flow. In short, I'd like to do it, but I am ever-so-slightly apprehensive about it like Ilya (mostly because after using Zizmor, I realized how fundamentally insecure GHA really is.) But it would let us cut releases quickly and easily, which does have value.

[kastl-ars]

(Johannes' work alter-ego writing)

Hi everyone,

thanks for giving the release process so much thought, highly appreciated.

In the meantime I would really like to get the security fix into openSUSE, so could someone point me in the right direction regarding the "skipping" of the failing tests that @arxanas mentioned?

I've seen packagers in certain other situations simply mark tests as "skipped" when they weren't compatible with the environment, for various reasons.

[kastl-ars]

I got the security fix in by completely disabling the checks, as I did not find a valid way of only disabling the gpgme checks.

Any hints would be highly appreciated...

[emilazy]

See https://nexte.st/docs/filtersets/ for test exclusion if you’re using the cargo nextest runner we use upstream, or I believe --skip=… works with the stock runner. It should work the same as any other Rust package.

But ideally we should fix the tests, if you are indeed supplying it with gpgsm and they’re still failing. Unfortunately our current test error output isn’t really sufficient to diagnose it.

[kastl-ars]

See https://nexte.st/docs/filtersets/ for test exclusion if you’re using the cargo nextest runner we use upstream, or I believe --skip=… works with the stock runner. It should work the same as any other Rust package.

What would the exact syntax be to "name the tests"? I tried something like --exclude 'test_gpg::gpgsm_signing_roundtrip'.

But ideally we should fix the tests, if you are indeed supplying it with gpgsm and they’re still failing. Unfortunately our current test error output isn’t really sufficient to diagnose it.

That would of course be optimal. Please reach out if I can be of assistance.

[kastl-ars]

OK, this syntax works (note the extra -- inbetween):

%{cargo_test} -- --skip 'test_gpg::gpgsm_signing_roundtrip' --skip 'test_gpg::gpgsm_signing_roundtrip_explicit_key' --skip 'test_gpg::gpgsm_unknown_key'

Should we rename this issue to make it clear that this is about the gpgme checks failing?

[mcepl]

It would be probably a good idea to add openSUSE to the list of distributions providing binaries in https://jj-vcs.github.io/jj/latest/install-and-setup/. The command is sudo zypper install jujutsu.