Spec URL: https://passt.top/static/rust-neli-proc-macros.spec SRPM URL: https://passt.top/static/rust-neli-proc-macros-0.1.3-1.fc41.src.rpm Description: Procedural macros for neli Fedora Account System Username: sbrivio
Copr build: https://copr.fedorainfracloud.org/coprs/build/8324348 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2329411-rust-neli-proc-macros/fedora-rawhide-x86_64/08324348-rust-neli-proc-macros/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.
plese add Source1: https://github.com/jbaublitz/neli/blob/main/LICENSE and %license %{SOURCE1} in %files
(In reply to solomoncyj from comment #2) > plese add Source1: https://github.com/jbaublitz/neli/blob/main/LICENSE > and %license %{SOURCE1} in %files Thanks for your review! Right, I saw that #FIXME but I wasn't sure how to fix it, and then I forgot... I fixed it like you suggested now, and updated SRPM and spec file.
[fedora-review-service-build]
Copr build: https://copr.fedorainfracloud.org/coprs/build/8325623 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2329411-rust-neli-proc-macros/fedora-rawhide-x86_64/08325623-rust-neli-proc-macros/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.
Taking this review. ============================================================ Some issues: 1. The SRPM and spec file don't match, please make sure to always upload / link the new versions of both files when making changes. 2. Mixed usage of spaces + tabs. TL;DR: Don't use tabs for indenting - rust2rpm defaults to spaces. 3. Don't point Source URLs at files from branches - these links are not stable. Point at a specific tag, if possible - if upstream does not tag releases in git, point at the commit that matches the packaged version. 4. The Source URL for the LICENSE file is wrong. It points at a view of the LICENSE file in the GitHub UI (i.e. an HTML page). Replace "/blob/" with "/raw/" to get a URL that points at the actual raw file contents. 5. Please ask upstream to release a new version of the neli-proc-macros crate. It looks like they made a change to include the LICENSE file (here: https://github.com/jbaublitz/neli/commit/e67aede) but never actually published a version with this change. 6. This project still uses version 1 of the "syn" crate. This version has been obsolete for almost two years at this point, and it will likely fail to parse Rust code that is valid syntax in future Rust versions. Please poke upstream project to port to "syn" v2. In most cases, this should be easy and require little to no code changes.
(In reply to Fabio Valentini from comment #6) > Taking this review. Thanks a lot! I'm addressing your comments. I'm currently at point 6.: > 6. This project still uses version 1 of the "syn" crate. This version has > been obsolete for almost two years at this point, and it will likely fail to > parse Rust code that is valid syntax in future Rust versions. Please poke > upstream project to port to "syn" v2. In most cases, this should be easy and > require little to no code changes. ...this one is taking a bit more effort than expected. With syn 2.0.90: -- Compiling neli-proc-macros v0.2.0-rc2 (/home/sbrivio/neli/neli-proc-macros) error[E0432]: unresolved imports `syn::token::Add`, `syn::token::Colon2`, `syn::NestedMeta` --> src/shared.rs:9:13 | 9 | token::{Add, Colon2}, | ^^^ ^^^^^^ no `Colon2` in `token` | | | no `Add` in `token` 10 | Attribute, Expr, Fields, FieldsNamed, FieldsUnnamed, GenericParam, Generics, Ident, Index, 11 | ItemStruct, Lit, Meta, MetaNameValue, NestedMeta, Path, PathArguments, PathSegment, Token, | ^^^^^^^^^^ no `NestedMeta` in the root | = help: consider importing one of these items instead: std::ops::Add syn::BinOp::Add help: a similar name exists in the module | 9 | token::{And, Colon2}, | ~~~ help: a similar name exists in the module | 9 | token::{Add, Colon}, | ~~~~~ error[E0599]: no method named `parse_meta` found for reference `&Attribute` in the current scope --> src/shared.rs:227:36 | 227 | if let Ok(meta) = attr.parse_meta() { | ^^^^^^^^^^ | help: there is a method `parse_nested_meta` with a similar name, but with different arguments --> /home/sbrivio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/syn-2.0.90/src/attr.rs:391:5 | 391 | / pub fn parse_nested_meta( 392 | | &self, 393 | | logic: impl FnMut(ParseNestedMeta) -> Result<()>, 394 | | ) -> Result<()> { | |___________________^ error[E0609]: no field `tokens` on type `&Attribute` --> src/shared.rs:235:70 | 235 | panic!("Could not parse provided attribute {}", attr.tokens,) | ^^^^^^ unknown field | = note: available fields are: `pound_token`, `style`, `bracket_token`, `meta` error[E0599]: no method named `parse_meta` found for struct `Attribute` in the current scope --> src/shared.rs:281:18 | 281 | attr.parse_meta() | ^^^^^^^^^^ | help: there is a method `parse_nested_meta` with a similar name, but with different arguments --> /home/sbrivio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/syn-2.0.90/src/attr.rs:391:5 | 391 | / pub fn parse_nested_meta( 392 | | &self, 393 | | logic: impl FnMut(ParseNestedMeta) -> Result<()>, 394 | | ) -> Result<()> { | |___________________^ error[E0609]: no field `tokens` on type `Attribute` --> src/shared.rs:282:81 | 282 | .unwrap_or_else(|_| panic!("Failed to parse attribute {}", attr.tokens)) | ^^^^^^ unknown field | = note: available fields are: `pound_token`, `style`, `bracket_token`, `meta` error[E0599]: no method named `parse_meta` found for reference `&Attribute` in the current scope --> src/shared.rs:376:14 | 375 | let meta = attr | ____________________- 376 | | .parse_meta() | |_____________-^^^^^^^^^^ | help: there is a method `parse_nested_meta` with a similar name, but with different arguments --> /home/sbrivio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/syn-2.0.90/src/attr.rs:391:5 | 391 | / pub fn parse_nested_meta( 392 | | &self, 393 | | logic: impl FnMut(ParseNestedMeta) -> Result<()>, 394 | | ) -> Result<()> { | |___________________^ error[E0609]: no field `tokens` on type `&Attribute` --> src/shared.rs:377:77 | 377 | .unwrap_or_else(|_| panic!("Failed to parse attribute {}", attr.tokens)); | ^^^^^^ unknown field | = note: available fields are: `pound_token`, `style`, `bracket_token`, `meta` error[E0609]: no field `nested` on type `MetaList` --> src/shared.rs:380:36 | 380 | for nested in list.nested { | ^^^^^^ unknown field | = note: available fields are: `path`, `delimiter`, `tokens` error[E0599]: no method named `parse_meta` found for reference `&Attribute` in the current scope --> src/shared.rs:407:14 | 406 | let meta = attr | ____________________- 407 | | .parse_meta() | |_____________-^^^^^^^^^^ | help: there is a method `parse_nested_meta` with a similar name, but with different arguments --> /home/sbrivio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/syn-2.0.90/src/attr.rs:391:5 | 391 | / pub fn parse_nested_meta( 392 | | &self, 393 | | logic: impl FnMut(ParseNestedMeta) -> Result<()>, 394 | | ) -> Result<()> { | |___________________^ error[E0609]: no field `tokens` on type `&Attribute` --> src/shared.rs:408:77 | 408 | .unwrap_or_else(|_| panic!("Failed to parse attribute {}", attr.tokens)); | ^^^^^^ unknown field | = note: available fields are: `pound_token`, `style`, `bracket_token`, `meta` error[E0609]: no field `nested` on type `MetaList` --> src/shared.rs:411:36 | 411 | for nested in list.nested { | ^^^^^^ unknown field | = note: available fields are: `path`, `delimiter`, `tokens` error[E0026]: struct `MetaNameValue` does not have a field named `lit` --> src/shared.rs:414:25 | 414 | lit: Lit::Str(lit), | ^^^ struct `MetaNameValue` does not have this field error[E0609]: no field `lit` on type `MetaNameValue` --> src/neli_enum.rs:13:38 | 13 | if let Lit::Str(ls) = nv.lit { | ^^^ unknown field | = note: available fields are: `path`, `eq_token`, `value` Some errors have detailed explanations: E0026, E0432, E0599, E0609. For more information about an error, try `rustc --explain E0026`. error: could not compile `neli-proc-macros` (lib) due to 13 previous errors -- ...I'm currently figuring out how to fix those so that I can propose the change upstream.
(In reply to Stefano Brivio from comment #7) > (In reply to Fabio Valentini from comment #6) > > Taking this review. > > Thanks a lot! I'm addressing your comments. I'm currently at point 6.: > > > 6. This project still uses version 1 of the "syn" crate. This version has > > been obsolete for almost two years at this point, and it will likely fail to > > parse Rust code that is valid syntax in future Rust versions. Please poke > > upstream project to port to "syn" v2. In most cases, this should be easy and > > require little to no code changes. > > ...this one is taking a bit more effort than expected. With syn 2.0.90: > > [...] > > ...I'm currently figuring out how to fix those so that I can propose the > change upstream. Then I found https://github.com/jbaublitz/neli/pull/241.
Thank you for taking a look! I read the upstream issue (and left a comment), and in this case, it doesn't look trivial to port from syn v1 to v2 at all, so adding this package as-is should be fine (so you can strike bullet point 6. from your TODO list for now).
(In reply to Fabio Valentini from comment #6) > Taking this review. > > ============================================================ > > Some issues: > > 1. The SRPM and spec file don't match, please make sure to always upload / > link the new versions of both files when making changes. Oops. Uploaded both now. > 2. Mixed usage of spaces + tabs. TL;DR: Don't use tabs for indenting - > rust2rpm defaults to spaces. Sorry, I'm a right-minded C programmer with strong moral values. Fixed. > 3. Don't point Source URLs at files from branches - these links are not > stable. Point at a specific tag, if possible - if upstream does not tag > releases in git, point at the commit that matches the packaged version. > > 4. The Source URL for the LICENSE file is wrong. It points at a view of the > LICENSE file in the GitHub UI (i.e. an HTML page). Replace "/blob/" with > "/raw/" to get a URL that points at the actual raw file contents. > > 5. Please ask upstream to release a new version of the neli-proc-macros > crate. It looks like they made a change to include the LICENSE file (here: > https://github.com/jbaublitz/neli/commit/e67aede) but never actually > published a version with this change. There's a new version now, so I could actually fix these three points by simply re-running rust2rpm. I just needed a small tweak to avoid a "File listed twice" warning (https://github.com/rpm-software-management/rpm/issues/336). > 6. This project still uses version 1 of the "syn" crate. This version has > been obsolete for almost two years at this point, and it will likely fail to > parse Rust code that is valid syntax in future Rust versions. Please poke > upstream project to port to "syn" v2. In most cases, this should be easy and > require little to no code changes. That happened at https://github.com/jbaublitz/neli/pull/256, and I bumped the version in the spec file. Spec URL: https://passt.top/static/rust-neli-proc-macros.spec SRPM URL: https://passt.top/static/rust-neli-proc-macros-0.2.0~rc3-1.fc42.src.rpm Description: Procedural macros for neli Fedora Account System Username: sbrivio
Created attachment 2066494 [details] The .spec file difference from Copr build 8325623 to 8528891
Copr build: https://copr.fedorainfracloud.org/coprs/build/8528891 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2329411-rust-neli-proc-macros/fedora-rawhide-x86_64/08528891-rust-neli-proc-macros/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.
> There's a new version now, so I could actually fix these three points by > simply re-running rust2rpm. I just needed a small tweak to avoid a "File > listed twice" warning > (https://github.com/rpm-software-management/rpm/issues/336). You don't need to "tweak" anything. The warning is harmless, and what rust2rpm generates is correct. Please revert that "tweak", which actually causes the file to be *included* twice, instead of harmlessly being *listed* twice :)
(In reply to Fabio Valentini from comment #13) > > There's a new version now, so I could actually fix these three points by > > simply re-running rust2rpm. I just needed a small tweak to avoid a "File > > listed twice" warning > > (https://github.com/rpm-software-management/rpm/issues/336). > > You don't need to "tweak" anything. The warning is harmless, and what > rust2rpm generates is correct. Please revert that "tweak", which actually > causes the file to be *included* twice, instead of harmlessly being *listed* > twice :) Oops. :) I guess rpm -q --licensefiles as I did is not a conclusive test. Spec URL: https://passt.top/static/rust-neli-proc-macros.spec SRPM URL: https://passt.top/static/rust-neli-proc-macros-0.2.0~rc3-1.fc42.src.rpm Description: Procedural macros for neli Fedora Account System Username: sbrivio
Created attachment 2066555 [details] The .spec file difference from Copr build 8528891 to 8537477
Copr build: https://copr.fedorainfracloud.org/coprs/build/8537477 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2329411-rust-neli-proc-macros/fedora-rawhide-x86_64/08537477-rust-neli-proc-macros/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.
Thank you for the update! I see that you're now packaging a pre-release / release candidate of v0.2.0. Note that we usually try to avoid this, since updates for pre-releases can contain breaking changes, but in this case I think it should be fine. === Package was generated with rust2rpm, simplifying the review. ✅ package contains only permissible content ✅ package builds and installs without errors on rawhide ✅ test suite is run and all unit tests pass ✅ latest version of the crate is packaged ✅ license matches upstream specification and is acceptable for Fedora ✅ license file is included with %license in %files ✅ package complies with Rust Packaging Guidelines Package APPROVED. === Recommended post-import rust-sig tasks: - set up package on release-monitoring.org: project: $crate homepage: https://crates.io/crates/$crate backend: crates.io version scheme: semantic version filter (*NOT* pre-release filter): alpha;beta;rc;pre distro: Fedora Package: rust-$crate - add @rust-sig with "commit" access as package co-maintainer (should happen automatically) - set bugzilla assignee overrides to @rust-sig (optional) - track package in koschei for all built branches (should happen automatically once rust-sig is co-maintainer)
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-neli-proc-macros
FEDORA-2025-121a3b6f11 (rust-neli-proc-macros-0.2.0~rc3-1.fc42) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2025-121a3b6f11
FEDORA-2025-121a3b6f11 (rust-neli-proc-macros-0.2.0~rc3-1.fc42) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.