Bug 2266544

1. Please address the FIXMEs in the spec file.
2. Don't disable the mangle_shebangs brp, fix the underlying issue (i.e. removing executable bits from all *.rs files).

I'll do a full review once these basics are addressed.

(In reply to Fabio Valentini from comment #1)
> 1. Please address the FIXMEs in the spec file.

Sorry, I overlooked that.

> 2. Don't disable the mangle_shebangs brp, fix the underlying issue (i.e.
> removing executable bits from all *.rs files).

That was a good pointer. I didn't expect that this was tied to an executable bit on the .rs file

> I'll do a full review once these basics are addressed.

I uploaded the two files (spec, srpm) again and also triggered a new scratch build (https://koji.fedoraproject.org/koji/taskinfo?taskID=114181652). The local mockbuild was fine.

Anything more you want me to do?

> That was a good pointer. I didn't expect that this was tied to an executable bit on the .rs file

Some editors "helpfully" see Rust files that start with "#![some_global_attribute]" and think "this is a shebang, let me mark the file as executable!" even though this is just valid Rust syntax and does not make the file executable at all.

> Anything more you want me to do?

Not right now. I will come back to this review ASAP.

Ok, I have some comments.

==========

> # disable i686 due to https://bugzilla.redhat.com/show_bug.cgi?id=2266232
> ExcludeArch: %{ix86}

You do not need an explanation for disabling i686.
For new packages (and existing leaf packages), it can just be dropped (and explicitly removing it is even encouraged):
https://fedoraproject.org/wiki/Changes/EncourageI686LeafRemoval

==========

> # prevent library files from being installed
> %global __cargo_is_lib() 0

This was an ugly workaround for cargo-rpm-macros < 26 that only worked "by accident".
With version 26, the correct form is now this:

"%global cargo_install_lib 0"

which of course also requires:

BuildRequires:  cargo-rpm-macros >= 26

This version of rust-packaging / cargo-rpm-macros is now available across all branches of Fedora and on EPEL9.

==========

The License tag can be simplified a bit. The SPDX "AND" and "OR" are commutative and associative (but not distributive), so some amount of duplication can be removed ("A OR B" and "B OR A" are equivalent, for example), and some nested parentheses can be dropped.

As far as I can tell, the resulting "simplified" license tag would look like this:

License:        Apache-2.0 AND BSD-3-Clause AND CC0-1.0 AND ISC AND MIT AND MPL-2.0 AND (0BSD OR MIT OR Apache-2.0) AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR ISC OR MIT) AND (Apache-2.0 OR MIT) AND (Apache-2.0 OR MIT OR MPL-2.0) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (CC0-1.0 OR MIT-0 OR Apache-2.0) AND (MIT OR Apache-2.0 OR Zlib) AND (Unlicense OR MIT)

Which is about 140 characters shorter.

==========

# the following tests don't work in the rpm build
%cargo_test -- --bin trunk -- --skip config::models_test::err_bad_trunk_toml_watch_ignore --skip config::models_test::err_bad_trunk_toml_watch_path --skip tools::tests::download_and_install_binaries

I would appreaciate something more detailed than just "these tests don't work":

- Are they missing data files that are not included in tarballs?
- Do they require internet access?

==========

Some licenses for bundled crates don't match with the licenses they are assigned in Fedora packages according to the rules we got from Red Hat Legal. For example, the regex-automata crate bundles some data from Unicode, which is licensed under the terms of the Unicode-DFS-2016 license - but this is not reflected in crate metadata.

Another example is the bundled "ring" crate. It does not specify an SPDX expression for its license at all, so it does not show up in the output of the %cargo_license / %cargo_license_summary macros. According to at two independent analyses (one by me and one by the authors of the "cargo-deny" tool, I think), the license expression for the "ring" crate would be best expressed as "ISC AND MIT AND OpenSSL". So the OpenSSL license is missing from the package's license tag, too.

==========

It might or might not be desirable for you to ship a nonstandard TLS implementation here, especially if you also want to ship this package in RHEL. I'm not sure if "ring" / "rustls" are an acceptable crypto stack there.

Since rustls / ring are not direct dependencies, but rather only pulled in because the "rustls" backend of several dependencies is chosen (axum, reqwest, tungstenite), it might be possible to switch to the "native-tls" backend (i.e. OpenSSL) with a patch.

==========

On that note - do you have a list of dependencies that would be missing if you were to try to build this package without vendored dependencies? Looking at the list of bundled crates, I don't see that many that are not packaged yet for Fedora (though some might be at the wrong version).

Packaging this initially with vendored dependencies would be fine to get things going, but it would be great if things could be unbundled on the short-to-medium-term.

==========

Other than these issues, the package looks pretty good. It's mostly the spec file as generated by "rust2rpm --vendor", just with the necessary modifications t make it complete and valid.

Thanks a lot for the review, and for providing so many pointers on how to get those issues sorted out.

As you mentioned, it mostly comes from the `rust2rpm` tool anyway. I noticed that you assume that 26 is the most recent version, for Fedora 39 it still is 25. Thus the "old" version. But even that fix is easy.

About the vendoring, I don't have a list. I am not sure if there's a tool already to simply check this. What would be cool in any case would be to have the ability to consume what is present, and vendor the rest.

I might need a bit more time to work through all of those, (especially the openssl/native-tls stuff) but I will try. For the client side that's simple, but for the service side the native-tls wasn't possible IIRC. I need to check back what the issue was, but for the server side it was either pretty hard, or impossible to get rid of rustls. Thus going "all in" with rustls, as otherwise the dependency list would have grown even more and one would have had multiple TLS stacks in a single application.

So I'll ping you on the issue once I worked through those issues. Thanks again.

(In reply to Jens Reimann from comment #6)
> As you mentioned, it mostly comes from the `rust2rpm` tool anyway. I noticed
> that you assume that 26 is the most recent version, for Fedora 39 it still
> is 25. Thus the "old" version. But even that fix is easy.

No, I meant what I said. rust2rpm and cargo-rpm-macros are from different packages and are not necessary updated in sync.
rust-packaging / cargo-rpm-macros really is at v26 now, whereas I am still working on releasing rust2rpm v26.

> About the vendoring, I don't have a list. I am not sure if there's a tool
> already to simply check this. What would be cool in any case would be to
> have the ability to consume what is present, and vendor the rest.

This is not really possible with the way cargo treats crate sources.
You can only specify *one* registry replacement. Doing partial vendoring is not feasible.

> I might need a bit more time to work through all of those, (especially the
> openssl/native-tls stuff) but I will try. For the client side that's simple,
> but for the service side the native-tls wasn't possible IIRC. I need to
> check back what the issue was, but for the server side it was either pretty
> hard, or impossible to get rid of rustls. Thus going "all in" with rustls,
> as otherwise the dependency list would have grown even more and one would
> have had multiple TLS stacks in a single application.

Ok, makes sense to me. Thank you for checking! Then sticking with rustls / ring should be OK.

Ok, I think I got all of your remarks covered, I think. There's a new version of the spec file and source:

Spec: https://dentrassi.de/download/rust-trunk/rust-trunk.spec
SRPM: https://dentrassi.de/download/rust-trunk/rust-trunk-0.19.0~rc.2-1.fc41.src.rpm

The reason for the version change is that trunk 0.19.0 is soon to be released and I was able to add support for `native-tls` (and `openssl` on the server side). Which means that by setting a cargo feature one can switch from rustls to native-tls/openssl. That also removes the `ring` dependency.

I added the Unicode license, but I am just not sure how to process the rest. Ring and rustls are gone.

I don't have a list yet. I tried to just build non-vendored, but that wouldn't show a full report as, to my understanding, it wouldn't show transient dependencies that are missing. But I did get some pointers from a call today on how it could be possible to evaluate that in a more complete fashion. I'll let you know when/if I get that working.

I would appreciate another round of review, if that's ok for you.

(In reply to Jens Reimann from comment #8)
> Ok, I think I got all of your remarks covered, I think. There's a new
> version of the spec file and source:
> 
> Spec: https://dentrassi.de/download/rust-trunk/rust-trunk.spec
> SRPM:
> https://dentrassi.de/download/rust-trunk/rust-trunk-0.19.0~rc.2-1.fc41.src.
> rpm
> 
> The reason for the version change is that trunk 0.19.0 is soon to be
> released and I was able to add support for `native-tls` (and `openssl` on
> the server side). Which means that by setting a cargo feature one can switch
> from rustls to native-tls/openssl. That also removes the `ring` dependency.

Great! It looks like this change was only partially applied, though. You need to pass feature flags to *all* %cargo_* macros (except %cargo_prep), otherwise you will get inconsistent results. For example, by adding the flags to %cargo_build / %cargo_install but not to %cargo_license / %cargo_license_summary, both the license summary and the generated breakdown (LICENSE.dependencies) will not match what is actually built and shipped.

PS: v0.19.0 was released in the meantime.

> I added the Unicode license, but I am just not sure how to process the rest.
> Ring and rustls are gone.

I couldn't easily verify this from the files you uploaded, since LICENSE.dependencies still lists it when built as-is.
After modifying your spec file to apply the feature flags correctly to all macros, you are right, "ring" and "rustls" are gone.

> I don't have a list yet. I tried to just build non-vendored, but that
> wouldn't show a full report as, to my understanding, it wouldn't show
> transient dependencies that are missing. But I did get some pointers from a
> call today on how it could be possible to evaluate that in a more complete
> fashion. I'll let you know when/if I get that working.

True, by default, you would only get a first-level view of dependencies that are missing.

> I would appreciate another round of review, if that's ok for you.

Thanks for the update! I have another few suggestions:

1. You don't actually *need* to package this from the sources that are published on crates.io if you never expect "trunk" to provide a Rust library interface.

See guidelines for this case: https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_rust_applications_non_crates_io_crates

I see that some tests are also disabled because files are missing from the tarball published on crates.io?

You could package this as "trunk" (i.e. without "rust-" prefix) and package from the GitHub tarball instead. That might make some things easier (including if trunk ever splits itself into multiple crates or stops publishing on crates.io - i.e. starts being a cargo workspace).

The spec file would be very similar to what you have now (but a but simpler). You can try this by downloading the GitHub sources and running "rust2rpm ./trunk-0.19.0/Cargo.toml --vendor" to generate a "local project" spec file.

2. Some of your spec file additions / modifications look quite out of place.

- "BuildRequires: openssl-devel" was added between Summary and License tags, instead of ... where there was already another BuildRequires line.
I would also recommend to document that "openssl-devel" dependency is necessary because the package needs OpenSSL header files present to generate and build Rust bindings for OpenSSL.

- The "ExcludeArch: %{ix86}" was added at the top. This is usually just preceding the BuildRequires.

- Defining custom macros usually happens at the top of the spec file. I'm not sure why you added all the macros at the top except for _trunk_features, which is in a really odd place.

Thanks again for the review. I like the idea of having the package named `trunk`, I'll be working towards that.

> PS: v0.19.0 was released in the meantime.

I know, that was me :) Already updated the spec file.

When it comes to the positions of fields in the file, I had (still have) no idea what's the preference. I'll just move them around as you suggested.

I am struggling adding the build flags to other tasks though, adding them like this:

```
%build
%cargo_build %{_trunk_features}
%{cargo_license_summary} %{_trunk_features}
%{cargo_license} %{_trunk_features} > LICENSE.dependencies
%{cargo_vendor_manifest} %{_trunk_features}
```

I'll run into this:

```
/var/tmp/rpm-tmp.rflLYE: line 58: syntax error near unexpected token `-n'
error: Bad exit status from /var/tmp/rpm-tmp.rflLYE (%build)
```

Maybe it's something obvious that I miss, but I don't see it. I am using the same way as with the `%cargo_build` (and others).

(In reply to Jens Reimann from comment #10)
> Thanks again for the review. I like the idea of having the package named
> `trunk`, I'll be working towards that.
> 
> > PS: v0.19.0 was released in the meantime.
> 
> I know, that was me :) Already updated the spec file.

Great, thanks!

> When it comes to the positions of fields in the file, I had (still have) no
> idea what's the preference. I'll just move them around as you suggested.

:thumbsup:

> I am struggling adding the build flags to other tasks though, adding them
> like this:
> 
> ```
> %build
> %cargo_build %{_trunk_features}
> %{cargo_license_summary} %{_trunk_features}
> %{cargo_license} %{_trunk_features} > LICENSE.dependencies
> %{cargo_vendor_manifest} %{_trunk_features}
> ```
> 
> I'll run into this:
> 
> ```
> /var/tmp/rpm-tmp.rflLYE: line 58: syntax error near unexpected token `-n'
> error: Bad exit status from /var/tmp/rpm-tmp.rflLYE (%build)
> ```
> 
> Maybe it's something obvious that I miss, but I don't see it. I am using the
> same way as with the `%cargo_build` (and others).

Adding the arguments this way results in them being appended to the expanded macro instead of being treated as arguments to the macro. You need to do move the flags inside the curly braces for them to be treated as the actual macro arguments, like this:

```
> %build
%cargo_build %{_trunk_features}
%{cargo_license_summary %{_trunk_features}}
%{cargo_license %{_trunk_features}} > LICENSE.dependencies
%{cargo_vendor_manifest}
```

(And the "%cargo_vendor_manifest" macro does not accept any arguments, so don't pass the flags there.)

Thanks again for the help.

I think I got it now. There's a new package as it got renamed. Not sure that requires a new bugzilla issue. I'll share it anyway:

SRPM: https://dentrassi.de/download/trunk/trunk-0.19.0-1.fc41.src.rpm
Spec: https://dentrassi.de/download/trunk/trunk.spec
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=114839010

Using the git repo as a base, it now also is possible to execute most of the tests. Yay!

Looks good to me! I'm glad that this seems to be working better for this case.

You don't need to file a new request, the bug title can just be changed (which I will just do now).

Spec URL: https://dentrassi.de/download/trunk/trunk.spec
SRPM URL: https://dentrassi.de/download/trunk/trunk-0.19.0-1.fc41.src.rpm

The review service should pick this up, hopefully.

There's a few more small things, but those don't block the final review (which I will do after posting this comment):

==============================

1. These seem to be leftover from the previous version, and I don't think they are necessary anymore:

> %global crate trunk
> %global crate_version 0.19.0

The `%crate_version` macro is only necessary if it doesn't match the Version string (i.e. for SemVer prereleases).
So you could just replace occurrences of `%{crate}` with `%{name}` and `%{crate_version}` with `%{version}` now.

Side note: This only works by accident right now, because you would need to use `%{crate_version}` instead of `%{version}` in the %autosetup argument as well to make it work for cases where version != crate_version:

> %autosetup -n %{crate}-%{version} -p1 -a1

This should be (if you want to keep the separate %crate_version macro):

%autosetup -n %{crate}-%{crate_version} -p1 -a1

==============================

2. I don't think you need to pass the `--bin trunk` argument to `%cargo_test`.

It should not be necessary to specify running tests only for the "trunk" binary.
`cargo test` should do this by default if that is the only target available that has tests.

==============================

3. It would be great if there were a few more comments in the spec file for some things, notably:

> # Disable defaults (update check and rustls), enable native-tls/openssl
> %global _trunk_features -n -f native-tls

Maybe this would be better:

# global feature flags:
# * disable self-update check
# * switch crypto backend from Rustls to OpenSSL
%global _trunk_features -n -f native-tls

> License:        (...) AND Unicode-DFS-2016

Here it would be good to document why Unicode-DFS-2016 needs to be added (i.e. that some crates bundle Unicode data, but do not specify this in their license metadata), maybe like this:

# Unicode-DFS-2016: the regex-syntax crate bundles Unicode data
License:        (...) AND Unicode-DFS-2016

I think the only crate affected by this issue (i.e. crate bundles Unicode data but does not declare this in the metadata) that is also getting statically linked into /usr/bin/trunk is the regex-syntax crate. But this might change in the future, so please keep an eye on it (other crates that are affected by this issue are currently: bstr, tinystr, databake).

> BuildRequires:  openssl-devel

It would be great to document why this is necessary:

# the bundled openssl-sys crate requires OpenSSL headers to be present
BuildRequires:  openssl-devel

Package looks good to me! Full review is included below.

================================================================================

There are some rpmlint warnings that I think can all be ignored:

> trunk.x86_64: W: no-manual-page-for-binary trunk

I don't think upstream provides a manual page?

> trunk-debugsource.x86_64: E: files-duplicated-waste 664357

This is likely due to vendored dependencies being included multiple times in different versions, there is nothing you can do about that in packaging - only bothering your dependencies to update their dependencies. :)

> trunk.x86_64: W: crypto-policy-non-compliance-openssl /usr/bin/trunk SSL_CTX_set_cipher_list

This seems to be a false positive that I've seen in other projects as well. As far as I can tell, rpmlint just checks for references to that symbol, but they're present only in to the generated bindings even if they're not actually used.

================================================================================

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

===== MUST items =====

C/C++:
[-]: Provides: bundled(gnulib) in place as required.
[x]: Package does not contain kernel modules.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[x]: %build honors applicable compiler flags or justifies otherwise.
[-]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: The License field must be a valid SPDX expression.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 4748 bytes in 2 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: SourceX tarball generation or download is documented.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[!]: Spec file according to URL is the same as in SRPM.
     > False positive due to rpmautospec.
[x]: Rpmlint is run on debuginfo package(s).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.

================================================================================

There are some post-import steps that I recommend for Rust packages (or for new packages in general):

1. set up the project on release-monitoring.org so you get notifications for new releases

In this case, "GitHub" source with "trunk-rs/trunk" as owner/project field, and "Semantic" as versioning scheme should do the trick.

2. track the package in koschei (will happen automatically if you do 3.)

3. add the "rust-sig" group as co-maintainer with "commit" access

PS: When importing the package, please take care to import the spec file that is *unmodified* by rpmautospec (i.e. without preamble and with %autochangelog as the only entry under %changelog).

Ok, I uploaded a newer version. Working those additional comments: dropping crate and crate_version. I wanted to keep the latter originally. Then again it might bet better to stick to released versions.

> > trunk.x86_64: W: no-manual-page-for-binary trunk
> 
> I don't think upstream provides a manual page?

True, there is no man page.

As for the other two, I don't know. Sounds reasonable.

> PS: When importing the package, please take care to import the spec file that is *unmodified* by rpmautospec (i.e. without preamble and with %autochangelog as the only entry under %changelog).

Meaning, the original authored file?

(In reply to Jens Reimann from comment #16)
> Ok, I uploaded a newer version. Working those additional comments: dropping
> crate and crate_version. I wanted to keep the latter originally. Then again
> it might bet better to stick to released versions.

Sounds good to me.

> > > trunk.x86_64: W: no-manual-page-for-binary trunk
> > 
> > I don't think upstream provides a manual page?
> 
> True, there is no man page.

Then not shipping one is fine. :)

> As for the other two, I don't know. Sounds reasonable.
> 
> > PS: When importing the package, please take care to import the spec file that is *unmodified* by rpmautospec (i.e. without preamble and with %autochangelog as the only entry under %changelog).
> 
> Meaning, the original authored file?

Yes, the one *not* included in the SRPM that is generated by "fedpkg srpm" (i.e. the one without "GENERATED BY rpmautpspec" stuff).

> Yes, the one *not* included in the SRPM that is generated by "fedpkg srpm" (i.e. the one without "GENERATED BY rpmautpspec" stuff).

Ok, understood!

Anything else I need to do? Or just wait?

> Anything else I need to do? Or just wait?

Not sure what you mean?

The package is approved now, you can continue the process for adding it to Fedora.

The Pagure repository was created at https://src.fedoraproject.org/rpms/trunk

FEDORA-2024-1ca4931c00 (trunk-0.19.0-1.fc41) has been submitted as an update to Fedora 41.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-1ca4931c00

FEDORA-2024-1ca4931c00 (trunk-0.19.0-1.fc41) has been pushed to the Fedora 41 stable repository.
If problem still persists, please make note of it in this bug report.