https://bugzilla.redhat.com/show_bug.cgi?id=2292255 --- Comment #5 from Carl George 🤠 <carl@xxxxxxxxxx> --- The linked spec file appears to be the rendered output after rpmautospec. Make sure when importing to use a SRPM that preserves the %autorelease and %autochangelog macros. https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs ================================================================================ This spec file uses the %{name} macro in a few places in the spec file. Unlike something like %{version}, the name is not expected to change, so there isn't really value in using it in places like the systemd unit file name. It's a matter of taste and is non-blocking for the review, but I think in the spirit of legibility that you should just use the word tailscale instead of the %{name} macro. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_legibility ================================================================================ The License tag contains GPL-3.0. That SPDX identifier is deprecated and is causing an invalid license rpmlint warning. You probably want to use either GPL-3.0-only or GPL-3.0-or-later. ================================================================================ This spec installs a sysconfig file. This type of file is a holdover from the sysvinit days, when it was necessary to change a daemon's command without editing the script itself. These days, systemd handles that much more gracefully with unit file overrides. My recommendation is to remove this file, the EnvironmentFile directive in the unit file, and just specify the port explicitly in the ExecStart line. This isn't forbidden in the guidelines, so this isn't a blocker to the review, just a suggestion. If you insist on keeping it, you'll need to at least fix the EnvironmentFile path in the unit file, as it currently reads /etc/default/tailscaled, while your file is installed to /etc/sysconfig/tailscaled. ================================================================================ There are files being removed in %prep with a comment about reducing the size of the SRPM. Those files are still contained in the tarball, so it isn't changing the size of the SRPM at all. If reducing the size is the goal, it would be better to move those steps into the tarball creation script. ================================================================================ This spec file contains both tailscale and tailscaled binaries. Should those be placed into separate subpackages? I'm not very familiar with how tailscale works, so disregard this if it is expected for both of these binaries to always be installed together. ================================================================================ The PATENTS and cmd/tailscale/cli/licenses.go files are not license files, and shouldn't be marked as such in %files. The PATENTS file may make sense as a %doc file, but the go source file probably shouldn't be included at all. There are also several files included as %doc that don't seem appropriate (k8s, sysv, derp, windows, etc). Please review these and only include relevant documentation. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation ================================================================================ The official RPMs from tailscale include the /var/cache/tailscale directory. The unit file creates that and /var/lib/tailscale. Both of these should probably be owned by the package. ================================================================================ How likely are you to switch this package from vendored deps to build requiring the dependencies? If you don't think it's likely to switch away from vendored deps anytime soon, I would remove those conditionals to make the spec more legible. ================================================================================ There is a comment in %build about downloading dependencies during the build. That is not permitted in Fedora's build system. As such it's likely unnecessary to set the GOPROXY environment variable. There is also a commented out GOSUMDB variable. If that is not needed then it would be better to remove it entirely instead of just commenting it out. ================================================================================ In %build there is a for loop being used to create two commands. Since it takes three lines to do this in a for loop, and just doing it directly would be two lines, why not keep it simple and drop the for loop? If you do keep the for loop, the usage of basename is unnecessary the way it is set up. ================================================================================ The way this is currently being built, the version subcommand has faulting output. 1.68.0-ERR-BuildInfo go version: go1.22.3 Upstream specficially requests that distributions build in a way that preserves the version info. https://github.com/tailscale/tailscale/blob/v1.68.1/README.md#building If I'm reading their source correctly, I think you can embed the version correctly by putting this in %build before the compilation. export LDFLAGS="-X tailscale.com/version.longStamp=%{version} -X tailscale.com/version.shortStamp=%{version}" ================================================================================ In %install, %gopkginstall should be conditionalized so that it doesn't run in a vendored build. Once you do this, you'll be able to remove the %exclude in %files, because those files are created by this macro. ================================================================================ It seems you have some extraneous scriptlet dependencies. You have a Requires(pre) dependency on shadow-utils, but you don't have a %pre scriptlet. The Requires(post), Requires(preun), and Requires(postun) dependencies on systemd are not necessary for the macros being called in those scriptlets. All of these should be removed. https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_dependencies_on_the_systemd_package -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2292255 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202292255%23c5 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
