@decathorpe the way that that rust2rpm has to patch out web-sys and wasm stuff surprises me. Can you confirm it is doing the right thing in this case?

This is correct, however you could likely drop the now-empty "web" feature as well.

The automatically dropped dependencies are only used on platforms where "all(target_arch = \"wasm32\", not(target_os = \"wasi\"), not(target_os = \"daku\"))" evaluates to true, which is ... on none of the target platforms we support.

However, since cargo dependency resolution is stupid and requires all dependencies to be present even if they end up being unused, we patch them out instead. (And this also means we don't have to package stuff that doesn't even work, like libraries that only target WebAssembly environments).

However, the bigger issue is the big "# FIXME: no license files detected" and bright red error message that should have been printed when you ran "rust2rpm" for this crate. :)

(In reply to Fabio Valentini from comment #2)
> This is correct, however you could likely drop the now-empty "web" feature
> as well.

You mean by manually making additional changes to the files generated by rust2rpm? Is there a good workflow to maintain such packages when we not just can run rust2rpm again in the future?

(Also, it seems like it could be a rust2rpm feature request to remove such empty features completely.)


Ok, I guess the license files is a blocker. Trying to resolve that upstream.


Meanwhile, an update with web feature removed:

Spec URL: https://kiilerix.fedorapeople.org/rust-whoami.spec
SRPM URL: https://kojipkgs.fedoraproject.org/work/tasks/535/102560535/rust-whoami-1.4.0-2.fc39.src.rpm

(In reply to Mads Kiilerich from comment #4)
> (In reply to Fabio Valentini from comment #2)
> > This is correct, however you could likely drop the now-empty "web" feature
> > as well.
> 
> You mean by manually making additional changes to the files generated by
> rust2rpm? Is there a good workflow to maintain such packages when we not
> just can run rust2rpm again in the future?

You can use "rust2rpm -p" to automate part of that workflow, or use a rust2rpm.conf configuration file to affect certain aspects of spec file generation that *don't* need manual interventions. In this case, you could set "unwanted-features = web", which would have the same effect.

However, I don't recommend using this setting in general, since it can have unintended consequences if not used right (but it would work in this case, since the "web" feature has no dependencies and is not depended on by any other features).

It looks like you've opted for option 3 (i.e. remove the "+web-devel" subpackage from the spec file, but not patch Cargo.toml. This is more or less equivalent to using the "unwanted-features" setting and *might or might not do what you expect depending on the circumstances*. I recommend patching out the "web" feature from Cargo.toml with "rust2rpm -p".

> (Also, it seems like it could be a rust2rpm feature request to remove such
> empty features completely.)

No can do. Features with no dependencies are valid. So removing any features that *end up empty after stripping unwanted dependencies* need to be handled on a case by case basis, and can't just be removed automatically, since that would be wrong in ~50% of cases.

> Ok, I guess the license files is a blocker. Trying to resolve that upstream.

Yes, for now. Thank you for approaching upstream.

> Meanwhile, an update with web feature removed:
> 
> Spec URL: https://kiilerix.fedorapeople.org/rust-whoami.spec
> SRPM URL:
> https://kojipkgs.fedoraproject.org/work/tasks/535/102560535/rust-whoami-1.4.
> 0-2.fc39.src.rpm

Thanks, the package looks good otherwise, I'll do the full review once the issue of the missing license texts is resolved.

(In reply to Fabio Valentini from comment #5)

Thaks for clarifying.

> It looks like you've opted for option 3 (i.e. remove the "+web-devel"
> subpackage from the spec file, but not patch Cargo.toml. This is more or
> less equivalent to using the "unwanted-features" setting and *might or might
> not do what you expect depending on the circumstances*. I recommend patching
> out the "web" feature from Cargo.toml with "rust2rpm -p".

No - I patch Cargo.toml too. I am editing whoami-fix-metadata-auto.diff . I guess that can be confusing if expecting that one to be owned by rust2rpm. But having multiple levels of patches is also confusing.

> Thanks, the package looks good otherwise, I'll do the full review once the
> issue of the missing license texts is resolved.

I fixed the license file issue upstream in 1.4.1, so it should be ready for review:

Spec URL: https://kiilerix.fedorapeople.org/rust-whoami.spec
SRPM URL: https://kojipkgs.fedoraproject.org/work/tasks/6265/102616265/rust-whoami-1.4.1-1.fc39.src.rpm

(In reply to Mads Kiilerich from comment #6)
> (In reply to Fabio Valentini from comment #5)
> 
> Thaks for clarifying.
> 
> > It looks like you've opted for option 3 (i.e. remove the "+web-devel"
> > subpackage from the spec file, but not patch Cargo.toml. This is more or
> > less equivalent to using the "unwanted-features" setting and *might or might
> > not do what you expect depending on the circumstances*. I recommend patching
> > out the "web" feature from Cargo.toml with "rust2rpm -p".
> 
> No - I patch Cargo.toml too. I am editing whoami-fix-metadata-auto.diff . I
> guess that can be confusing if expecting that one to be owned by rust2rpm.
> But having multiple levels of patches is also confusing.

That is indeed the *most* confusing option I've encountered yet ...
Especially because the filename contains "auto" and the comment in the spec file says "Automatically generated" ...
The "rust2rpm -p" workflow is intended to address this exact issue (even if it results in two levels of patch, one automatically generated that shouldn't be touched, one for manual changes).

> > Thanks, the package looks good otherwise, I'll do the full review once the
> > issue of the missing license texts is resolved.
> 
> I fixed the license file issue upstream in 1.4.1, so it should be ready for
> review:
> 
> Spec URL: https://kiilerix.fedorapeople.org/rust-whoami.spec
> SRPM URL:
> https://kojipkgs.fedoraproject.org/work/tasks/6265/102616265/rust-whoami-1.4.
> 1-1.fc39.src.rpm

Thanks, I'll review after I had lunch. :)

Package was generated with rust2rpm, simplifying the review.

- 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 files are included with %license in %files
- package complies with Rust Packaging Guidelines

Package APPROVED.

===

Recommended post-import rust-sig tasks:

- add @rust-sig with "commit" access as package co-maintainer

- set bugzilla assignee overrides to @rust-sig (optional)

- set up package on release-monitoring.org:
  project: $crate
  homepage: https://crates.io/crates/$crate
  backend: crates.io
  version scheme: semantic
  version filter: alpha;beta;rc;pre
  distro: Fedora
  Package: rust-$crate

- track package in koschei for all built branches

The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-whoami

Thanks for the review and approval. We are now ready for Mercurial 6.5 .

> Recommended post-import rust-sig tasks:
> 
> - add @rust-sig with "commit" access as package co-maintainer
> - set bugzilla assignee overrides to @rust-sig (optional)

It would be even more helpful for less experienced packagers like me to say:

- on https://src.fedoraproject.org/rpms/rust-$crate :
-- make the group "rust-sig" co-maintainers by giving "commit" access
-- edit "Bugzilla Assignee" to assign new bugs to "@rust-sig" (optional)

> - set up package on release-monitoring.org:

- add a project on release-monitoring.org to monitor the package:

> - track package in koschei for all built branches

- on https://koschei.fedoraproject.org/package/rust-$crate , do XXX to track package status on all built branches

Thanks, I'll try to update the template I use for this.

This was imported and built.