Spec URL: https://download.copr.fedorainfracloud.org/results/uril/trustee-attester/fedora-rawhide-x86_64/08272751-rust-jsonwebkey/rust-jsonwebkey.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/uril/trustee-attester/fedora-rawhide-x86_64/08272751-rust-jsonwebkey/rust-jsonwebkey-0.3.5-1.fc42.src.rpm Description: JSON Web Key (JWK) (de)serialization , and conversion. https://datatracker.ietf.org/doc/html/rfc7517 - Serialization and deserialization of Required and Recommended key types (HS256, RS256, ES256) - Conversion to PEM/DER Only 'pkcs-convert' feature is included - pkcs-convert - enables Key::{to_der, to_pem}. Fedora Account System Username: uril
Copr build: https://copr.fedorainfracloud.org/coprs/build/8275888 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2326979-rust-jsonwebkey/fedora-rawhide-x86_64/08275888-rust-jsonwebkey/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
So I attempted to reproduce spec with rust2rpm on fedora40 host. After doing the dance of applying the patch diff interactively, here's the diff in the spec file output: ``` --- rust-jsonwebkey.spec 2024-11-26 15:38:57.277635956 -0500 +++ bak 2024-11-26 15:26:49.190513593 -0500 @@ -2,6 +2,9 @@ %bcond_without check %global debug_package %{nil} +# prevent executables from being installed +%global cargo_install_bin 0 + %global crate jsonwebkey Name: rust-jsonwebkey @@ -15,7 +18,7 @@ Source: %{crates_source} # Manually created patch for downstream crate metadata changes Patch: jsonwebkey-fix-metadata.diff -BuildRequires: cargo-rpm-macros >= 24 +BuildRequires: cargo-rpm-macros >= 26 %global _description %{expand: JSON Web Key (JWK) (de)serialization, generation, and conversion.} @@ -36,42 +39,6 @@ use the "%{crate}" crate. %doc %{crate_instdir}/README.md %{crate_instdir}/ -%package -n %{name}+default-devel -Summary: %{summary} -BuildArch: noarch - -%description -n %{name}+default-devel %{_description} - -This package contains library source intended for building other packages which -use the "default" feature of the "%{crate}" crate. - -%files -n %{name}+default-devel -%ghost %{crate_instdir}/Cargo.toml - -%package -n %{name}+generate-devel -Summary: %{summary} -BuildArch: noarch - -%description -n %{name}+generate-devel %{_description} - -This package contains library source intended for building other packages which -use the "generate" feature of the "%{crate}" crate. - -%files -n %{name}+generate-devel -%ghost %{crate_instdir}/Cargo.toml - -%package -n %{name}+jwt-convert-devel -Summary: %{summary} -BuildArch: noarch - -%description -n %{name}+jwt-convert-devel %{_description} - -This package contains library source intended for building other packages which -use the "jwt-convert" feature of the "%{crate}" crate. - -%files -n %{name}+jwt-convert-devel -%ghost %{crate_instdir}/Cargo.toml - %package -n %{name}+pkcs-convert-devel Summary: %{summary} BuildArch: noarch ``` Were those latter bits manually deleted? Or did I miss something in the invocation? Notably it seemed odd to me that `-n %{name}+default-devel` subpackage was missing since that seems to be in every rust crate I've looked at (but maybe I'm missing something). I think it would also be helpful to enumerate in the `Patch:` comment what deps were tweaked. Like 'adjust version for crate FOO and BAR to match Fedora. drop BAZ and WIBBLE which aren't packaged. drop all deps for BLAH feature which we don't need' or something to that effect.
I agree, the changes made to the package look strange. The patch does some things that I would classify as "suspicious": 1. Bumping the bitflags dependency from ^1.2 to ^1.3 - this looks like a misunderstanding of what `version = "1.2"` means (it means "1.2 or any version compatible with it, i.e. <2.0", which includes 1.3.x) 2. Removing the dependencies for the "generate" and "jwt-convert" features, but not the features themselves is wrong. If these features should not be available in the packaged crate, just dropping the dependencies but not the features themselves will have unintentional consequences (for example, the subpackages for these features will still be generated in the .spec file). 3. Moving "num-bigint" and "yasna" into "named dependencies" (i.e. "dep:num-bigint" and "dep:yasna") is an unnecessary divergence from the upstream crate metadata, and could lead to problems with dependent crates which could expect the implicitly defined features for "num-bigint" and "yasna" to be present. 4. Setting `%global cargo_install_bin 0` is a noop, this crate does not contain any executables. I don't know where you got this from, but it is useless here. 5. Do not drop the rust-%{crate}-$feature-devel subpackages from the generated spec file manually. They are there for a reason, and they MUST match cargo metadata to avoid build failures or unexpected problems in packages for dependent crates. In particular, dropping the subpackage for the "default" feature is problematic. The "default" feature is unconditionally defined by cargo even if it is not explicitly mentioned in crate metadata. In general, I would recommend not to make any changes to the spec file generated by rust2rpm unless you are *sure* that they are correct and necessary.
(In reply to Cole Robinson from comment #2) Cole, thank you for reviewing. > So I attempted to reproduce spec with rust2rpm on fedora40 host. After doing > the dance of applying the patch diff interactively, here's the diff in the > spec file output: <rust-jsonwebkey.spec diff dropped> > Were those latter bits manually deleted? Or did I miss something in the > invocation? The spec file was created with rust2rpm.toml: -------------- [package] cargo-install-bin = false cargo-install-lib = true [features] hide = ["generate", "jwt-convert", "default"] -------------- Next time I'll add the rust2rpm.toml as a comment/attachment. > Notably it seemed odd to me that `-n %{name}+default-devel` subpackage was > missing > since that seems to be in every rust crate I've looked at (but maybe I'm > missing something). All the "features" except for "pkcs-convert" are "hidden". I'll "unhide" the "default" feature. > > I think it would also be helpful to enumerate in the `Patch:` comment what > deps were tweaked. Like 'adjust version for crate FOO and BAR to match > Fedora. drop BAZ and WIBBLE which aren't packaged. drop all deps for BLAH > feature which we don't need' or something to that effect. Yeah, I'll add such comments.
(In reply to Uri Lublin from comment #4) > > The spec file was created with rust2rpm.toml: > -------------- > [package] > cargo-install-bin = false > cargo-install-lib = true > > [features] > hide = ["generate", "jwt-convert", "default"] > -------------- The cargo-install-bin and cargo-install-lib values set here are the defaults for crates like this one, making them explicit only adds noise to the spec file. I would strongly recommend to drop the [package] section entirely.
(In reply to Fabio Valentini from comment #3) Fabio, thank you for looking too. I added the rust2rpm.toml used in comment 4. > I agree, the changes made to the package look strange. > > The patch does some things that I would classify as "suspicious": > > 1. Bumping the bitflags dependency from ^1.2 to ^1.3 - this looks like a > misunderstanding of what `version = "1.2"` means (it means "1.2 or any > version compatible with it, i.e. <2.0", which includes 1.3.x) Okay, I'll leave it at "1.2". > > 2. Removing the dependencies for the "generate" and "jwt-convert" features, > but not the features themselves is wrong. > If these features should not be available in the packaged crate, just > dropping the dependencies but not the features themselves will have > unintentional consequences (for example, the subpackages for these features > will still be generated in the .spec file). The features "generate" and "jwt-convert" are not needed and so are "hidden". Removing features in Cargo.toml can be risky, since then code with e.g. '#[cfg(feature = "<removed-feature>")] fails to build. If the dependencies of hidden features stay in Cargo.toml, they are still required. That can be a problem especially if they are not in Fedora. I found that having the "hidden" features in Cargo.toml but removing their dependencies solves it - the code builds, the hidden features do not become subpackages and their dependencies are not required. > > 3. Moving "num-bigint" and "yasna" into "named dependencies" (i.e. > "dep:num-bigint" and "dep:yasna") is an unnecessary divergence from the > upstream crate metadata, and could lead to problems with dependent crates > which could expect the implicitly defined features for "num-bigint" and > "yasna" to be present. I think there is a problem with rust2rpm. I am not sure so I did not yet report it. The problem is that, without changes, rust2rpm creates a spec file with subpackages for dependencies (not features). With the added "dep:" for all dependencies rust2rpm works correctly. For example, with this package (with the above rust2rpm.toml file): $ rust2rpm jsonwebkey 0.3.5 • Generated: rust-jsonwebkey.spec $ grep '^.package' rust-jsonwebkey.spec %package devel %package -n %{name}+jsonwebtoken-devel %package -n %{name}+num-bigint-devel %package -n %{name}+p256-devel %package -n %{name}+pkcs-convert-devel %package -n %{name}+rand-devel %package -n %{name}+yasna-devel > > 4. Setting `%global cargo_install_bin 0` is a noop, this crate does not > contain any executables. I don't know where you got this from, but it is > useless here. Okay, I can remove it, if you prefer. With it in the rust2rpm.toml file, I'm sure that even if there are binaries they are not included. > > 5. Do not drop the rust-%{crate}-$feature-devel subpackages from the > generated spec file manually. They are there for a reason, and they MUST > match cargo metadata to avoid build failures or unexpected problems in > packages for dependent crates. > > In particular, dropping the subpackage for the "default" feature is > problematic. The "default" feature is unconditionally defined by cargo even > if it is not explicitly mentioned in crate metadata. I'll "unhide" the "default" feature. > > In general, I would recommend not to make any changes to the spec file > generated by rust2rpm unless you are *sure* that they are correct and > necessary.
(In reply to Fabio Valentini from comment #5) > (In reply to Uri Lublin from comment #4) > > > > The spec file was created with rust2rpm.toml: > > -------------- > > [package] > > cargo-install-bin = false > > cargo-install-lib = true > > > > [features] > > hide = ["generate", "jwt-convert", "default"] > > -------------- > > The cargo-install-bin and cargo-install-lib values set here are the defaults > for crates like this one, > making them explicit only adds noise to the spec file. I would strongly > recommend to drop the [package] section entirely. Okay, I'll remove them. Thanks.
New build: Spec URL: https://download.copr.fedorainfracloud.org/results/uril/trustee-attester/fedora-rawhide-x86_64/08324061-rust-jsonwebkey/rust-jsonwebkey.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/uril/trustee-attester/fedora-rawhide-x86_64/08324061-rust-jsonwebkey/rust-jsonwebkey-0.3.5-1.fc42.src.rpm rust2rpm.toml: [features] hide = ["generate", "jwt-convert"]
Created attachment 2060222 [details] The .spec file difference from Copr build 8275888 to 8324085
Copr build: https://copr.fedorainfracloud.org/coprs/build/8324085 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2326979-rust-jsonwebkey/fedora-rawhide-x86_64/08324085-rust-jsonwebkey/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
This increasingly looks like you're fighting with cargo / rust2rpm, or a misunderstanding of how features / optional dependencies work in cargo, because the comments here are wrong: > # - Dependency jsonwebtoken removed as it's only > # required by hidden feature 'jwt-convert' > # - Dependencies p256 and rand were removed as they are only > # required by hidden feature 'generate' > # - "dep:" was added to all dependencies in [features] table as > # it seems this overcomes an issue with rust2rpm see also my comment on your bug report here: https://bugzilla.redhat.com/show_bug.cgi?id=2329304#c2 Also, the Cargo.toml patch is still wrong, and it still introduces an unnecessary divergence between what would be shipped in Fedora and the upstream crate from crates.io. In particular, this is wrong and will just cause problems: """ pkcs-convert = [ - "num-bigint", - "yasna", + "dep:num-bigint", + "dep:yasna", ] """ I would recommend to read the section about the "features.hide" setting in the man page for rust2rpm.toml. ============================================================ All in all, I'm still not sure what you're attempting to do (or why), so it might be a good idea to take a step back and ask: Why do you want to (or think you need to) disable the features for the "generate" and "jwt-convert" features in the package for this crate? 1. In the case of the "generate" feature, it looks like all its dependencies are already available in Fedora, but the crate just depends on a really old version of the "p256" crate. If you don't need the "generate" feature for what you're working on, then disabling it is a valid thing to do, but then the easiest solution would be to add ["generate", "p256", "rand"] to the features.hide setting in rust2rpm. 2. In the case of the "jwt-convert" feature, the jsonwebtoken dependency would also be available as a Fedora package, but "pkcs-convert" is not. If you don't need the "jwt-convert" feature, then adding all of ["jwt-convert", "jsonwebtoken", "pkcs-convert"] to the features.hide setting would be a valid thing to do. For both 1. and 2. the justification comments should be "disable feature because it pulls in unavailable and / or outdated dependencies". As far as I can tell, the patch for Cargo.toml that you need would be just these two changes: """ version = "1.0" [dependencies.yasna] -version = "0.4" +version = "0.5" features = ["num-bigint"] optional = true """ and """ version = "1.4" features = ["zeroize_derive"] -[dev-dependencies.jsonwebtoken] -version = "8.0" - [features] generate = [ """ in combination with this rust2rpm.toml config file: """ [package] cargo-toml-patch-comments = [ "bump yasna dependency from 0.4 to 0.5", "drop jsonwebtoken dev-dependency: only needed by tests for the disabled jwt-convert feature", ] [features] hide = [ # unused non-default "generate" feature # with outdated dependency on the "p256" crate "generate", "p256", "rand", # unused non-default "jwt-convert" feature # with outdated depepdency on the "jsonwebtoken" crate "jwt-convert", "pkcs-convert", "jsonwebtoken", ] """ For the yasna v0.4 -> v0.5 bump, it would be great if you could submit a PR to upstream: https://github.com/nhynes/jwk-rs Though it looks like this project has been abandoned two years ago (which explains the outdated dependencies), so I'm not sure how successful that would be.
Fabio, thank you for the detailed explanations and suggestions. I'll fix this package as you recommend. > Why do you want to (or think you need to) disable the features for the "generate" and "jwt-convert" features in the package for this crate? I do not need the features "generate" and "jwt-convert" and so I disabled them. I only need the "pkcs-convert" feature. I can keep "jwt-convert" if it's preferred - by updating jsonwebtoken version to "9" > For the yasna v0.4 -> v0.5 bump, it would be great if you could submit a PR to upstream: https://github.com/nhynes/jwk-rs I'll do that.
> > For the yasna v0.4 -> v0.5 bump, it would be great if you could submit a PR to upstream: > https://github.com/nhynes/jwk-rs > > I'll do that. It's already in upstream git repo; committed after version 0.3.5 was released
Spec URL: https://download.copr.fedorainfracloud.org/results/uril/trustee-attester/fedora-rawhide-x86_64/08333766-rust-jsonwebkey/rust-jsonwebkey.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/uril/trustee-attester/fedora-rawhide-x86_64/08333766-rust-jsonwebkey/rust-jsonwebkey-0.3.5-1.fc42.src.rpm rust2rpm.toml: [package] cargo-toml-patch-comments = [ "bump yasna dependency from 0.4 to 0.5", "drop jsonwebtoken dev-dependency: only needed by tests for the disabled jwt-convert feature", ] [features] hide = [ # unused non-default "generate" feature # with outdated dependency on the "p256" crate "generate", "p256", "rand", # unused non-default "jwt-convert" feature # with outdated dependency on the "jsonwebtoken" crate "jwt-convert", "jsonwebtoken", ]
Sorry, the Fedora Review Service build wasn't submitted because of a yesterday planned Copr outage. [fedora-review-service-build]
Created attachment 2061344 [details] The .spec file difference from Copr build 8324085 to 8344631
Copr build: https://copr.fedorainfracloud.org/coprs/build/8344631 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2326979-rust-jsonwebkey/fedora-rawhide-x86_64/08344631-rust-jsonwebkey/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Thanks for the education Fabio! I learned a few things :) SRPM contents match Fabio's suggestions now, everything else looks good to me. Setting fedora-review+ Uri I've sponsored you into the packager group now. You may need to fully toggle login at src.fedoraproject.org before you can follow rest of the steps to get the package repo created: https://docs.fedoraproject.org/en-US/package-maintainers/Package_Review_Process/#_contributor
Thank you so much Cole and thanks again Fabio !
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-jsonwebkey
FEDORA-2024-b685960370 (rust-jsonwebkey-0.3.5-1.fc42) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2024-b685960370
FEDORA-2024-b685960370 (rust-jsonwebkey-0.3.5-1.fc42) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.
It looks like you didn't add the rust2rpm.toml config file you used to the repo? I would recommend that you commit it into the dist-git repo so it doesn't get lost and / or other people can re-use it.