Spec URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/fedora-rawhide-x86_64/07612992-tailscale/tailscale.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/fedora-rawhide-x86_64/07612992-tailscale/tailscale-1.68.0-1.fc41.src.rpm Description: Tailscale VPN Client built around WireGuard Fedora Account System Username: jonathanspw
Copr build: https://copr.fedorainfracloud.org/coprs/build/7613040 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2292255-tailscale/fedora-rawhide-x86_64/07613040-tailscale/fedora-review/review.txt Found issues: - Not a valid SPDX expression '(MIT OR Apache-2.0) AND (BSD-2-Clause OR ISC) AND (MIT OR BSD-3-Clause OR Apache-2.0) AND (Apache-2.0 OR CC-BY-SA-4.0) AND (Apache-2.0 OR BSD-3-Clause) AND (BSD-2-Clause-Views OR BSD-3-Clause) AND BSD-2-Clause AND MIT AND (BSD-3-Clause OR Apache-2.0 OR MIT) AND GPL-3.0 AND Apache-2.0 AND MPL-2.0 AND (Apache-2.0 OR MIT) AND (GPL-3.0 OR GPL-3.0-or-later) AND (MIT OR BSD-3-Clause) AND ISC AND 0BSD AND BSD-3-Clause'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 - Systemd service file(s) in tailscale Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets Please know that there can be false-positives. --- 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.
Spec URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/fedora-rawhide-x86_64/07613074-tailscale/tailscale.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/fedora-rawhide-x86_64/07613074-tailscale/tailscale-1.68.0-1.fc41.src.rpm
Created attachment 2037181 [details] The .spec file difference from Copr build 7613040 to 7613139
Copr build: https://copr.fedorainfracloud.org/coprs/build/7613139 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2292255-tailscale/fedora-rawhide-x86_64/07613139-tailscale/fedora-review/review.txt Found issues: - Not a valid SPDX expression '(MIT OR Apache-2.0) AND (BSD-2-Clause OR ISC) AND (MIT OR BSD-3-Clause OR Apache-2.0) AND (Apache-2.0 OR CC-BY-SA-4.0) AND (Apache-2.0 OR BSD-3-Clause) AND (BSD-2-Clause-Views OR BSD-3-Clause) AND BSD-2-Clause AND MIT AND (BSD-3-Clause OR Apache-2.0 OR MIT) AND GPL-3.0 AND Apache-2.0 AND MPL-2.0 AND (Apache-2.0 OR MIT) AND (GPL-3.0 OR GPL-3.0-or-later) AND (MIT OR BSD-3-Clause) AND ISC AND 0BSD AND BSD-3-Clause'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 Please know that there can be false-positives. --- 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.
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
(In reply to Carl George 🤠 from comment #5) > ============================================================================= > === > > 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. > As this would be the only way to configure the daemon for various non-default modes, I agree with fixing the environmentfile path. > ============================================================================= > === > > 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. > tailscale is the client for the tailscaled daemon, so they should be 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 > It's probably fine to put PATENTS in licenses, but otherwise this is correct. > ============================================================================= > === > > 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}" > Please use "%{version}-%{release}" so that each build is easily identified.
> As this would be the only way to configure the daemon for various non-default modes, I agree with fixing the environmentfile path. It's not the only way to configure the daemon. It is a distro-specific legacy method for configuring the daemon. The modern cross-distro way to do this is with systemd unit file overrides. > It's probably fine to put PATENTS in licenses, but otherwise this is correct. My thinking on this goes back to the reason we have a %license attribute separate from %doc. A system can be configured to skip installing documentation files, but many licenses require the license text (or a subset of it) always be distrusted with the software. So a %license file is not skippable, but a %doc file is. I don't see any requirement in the upstream PATENTS file to distribute that file with the software, so I think it makes more sense as a %doc. That said, the PATENTS file does say the literal word "license", so perhaps it could go either way. I've seen it done both ways in other Fedora spec files. > Please use "%{version}-%{release}" so that each build is easily identified. I don't think that will work. Upstream does special parsing of the version strings for various properties of this output, so I believe including the release field will cause similar errors to what are there now (or cause the release field to be presented as something it isn't, like the commit hash).
Spec URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/centos-stream-9-aarch64/07621796-tailscale/tailscale.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/centos-stream-9-aarch64/07621796-tailscale/tailscale-1.68.1-1.el9.src.rpm Implemented all suggested changes but I left the patents file flagged as as license. Switching to non-vendored deps is something I hope to do but it will be a long road to get there as the tree is pretty insane. I'd prefer to keep it conditionalized for now.
Created attachment 2037615 [details] The .spec file difference from Copr build 7613139 to 7621835
Copr build: https://copr.fedorainfracloud.org/coprs/build/7621835 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2292255-tailscale/fedora-rawhide-x86_64/07621835-tailscale/fedora-review/review.txt Found issues: - Not a valid SPDX expression '(MIT OR Apache-2.0) AND (BSD-2-Clause OR ISC) AND (MIT OR BSD-3-Clause OR Apache-2.0) AND (Apache-2.0 OR CC-BY-SA-4.0) AND (Apache-2.0 OR BSD-3-Clause) AND (BSD-2-Clause-Views OR BSD-3-Clause) AND BSD-2-Clause AND MIT AND (BSD-3-Clause OR Apache-2.0 OR MIT) AND AND Apache-2.0 AND MPL-2.0 AND (Apache-2.0 OR MIT) AND GPL-3.0-or-later AND (MIT OR BSD-3-Clause) AND ISC AND 0BSD AND BSD-3-Clause'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 Please know that there can be false-positives. --- 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.
Your license expression needs some adjustments. At one point it has "AND AND", which results in an rpmlint warning. You have a couple of duplicative OR sub-expressions that are the same thing in a different order. - (MIT OR Apache-2.0) - (Apache-2.0 OR MIT) - (MIT OR BSD-3-Clause OR Apache-2.0) - (BSD-3-Clause OR Apache-2.0 OR MIT) You can actually drop the OR sub-expressions that already fully covered by the other licenses outside the sub-expressions. https://docs.fedoraproject.org/en-US/legal/license-field/#_special_rules_for_or_expressions Based on the guidelines, this should be the correct license tag, although you may want to look closer into that "AND AND" string to make sure no licenses were missed. License: 0BSD AND Apache-2.0 AND BSD-2-Clause AND BSD-3-Clause AND GPL-3.0-or-later AND ISC AND MIT AND MPL-2.0 AND (Apache-2.0 OR CC-BY-SA-4.0) AND (BSD-2-Clause-Views OR BSD-3-Clause) ================================================================================ You still have cmd/tailscale/cli/licenses.go included as a %license file. It isn't a license file, it's a source file related to the licenses subcommand. It needs to be removed. ================================================================================ There is still irrelevant documentation included. At a minimum, docs/sysv/ and docs/windows/ are definitely not relevant and need to be removed. I'll leave it up to you if derp/README.md, docs/bird/, docs/k8s/, and docs/webhooks/ are relevant enough to be included (my guess is no). ================================================================================ %gopkginstall is in both %build and %install. It should only be in %install. On a related note, the %gobuild commands in %build are inside an %else that will cause them to not be run if you ever disable the vendor conditional. That %else conditional should be removed so that the %gobuild commands always run. ================================================================================ In your unit file, you're referencing /usr/sbin/tailscaled, but you're installing the command as /usr/bin/tailscaled. These should match. Considering the upcoming change to unify /usr/bin and /usr/sbin, the best course of action would be to change the unit file to match /usr/bin/tailscaled. https://fedoraproject.org/wiki/Changes/Unify_bin_and_sbin ================================================================================ Is there a purpose to commenting out the GOPROXY line instead of removing it, along with the corresponding comments? ================================================================================ What is the benefit of specifying PORT and FLAGS as environment variables in the unit file? Now that you dropped the sysconfig file, they seem pointless to me. Since a user has to create a unit file override to modify them, they can just edit the ExecStart line directly at that point. For simplicity, I think this should just be: ExecStart=/usr/bin/tailscaled --state=/var/lib/tailscale/tailscaled.state --socket=/run/tailscale/tailscaled.sock --port=41641 ================================================================================ Currently the %check section is not being run. It is tied to whether or not you are doing a vendored build. In theory the tests should be able to be run regardless of whether you're doing a vendored build or not. Try to run these if you can get them to pass.
Spec URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/centos-stream-9-x86_64/07667481-tailscale/tailscale.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/jonathanspw/tailscale/centos-stream-9-x86_64/07667481-tailscale/tailscale-1.68.1-1.el9.src.rpm We can't run tests while using vendored deps as the vendoring doesn't grab all required deps for the tests to run. As for PORT/FLAGS I just left them separate to more closely follow upstream. It's mostly preferential.
Created attachment 2038300 [details] The .spec file difference from Copr build 7621835 to 7667504
Copr build: https://copr.fedorainfracloud.org/coprs/build/7667504 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2292255-tailscale/fedora-rawhide-x86_64/07667504-tailscale/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 license tag is closer, but I do see two more tweaks that could be made. - (BSD-2-Clause OR ISC) is already fully covered by the other licenses outside the sub-expressions, so it's unnecessary. - (Apache-2.0 OR CC-BY-SA-4.0) is referencing github.com/opencontainers/go-digest, which states in it's readme that only the docs are CC-BY-SA-4.0. Since you're only building against the code and not shipping the docs, this should just be Apache-2.0 (which is also already covered). But those changes are small enough you can make them after doing the import. This package is APPROVED. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== MUST items ===== 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]: 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. [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. [x]: 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. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10846 bytes in 4 files. [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. Note: There are rpmlint messages (see attachment). [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]: systemd_post is invoked in %post, systemd_preun in %preun, and systemd_postun in %postun for Systemd service files. Note: Systemd service file(s) in tailscale [x]: File names are valid UTF-8. [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). [x]: 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. Note: Package contains tarball without URL, check comments [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: %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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: SourceX is a working URL. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [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. [x]: Spec file according to URL is the same as in SRPM.
The Pagure repository was created at https://src.fedoraproject.org/rpms/tailscale
FEDORA-2024-81017eae46 (tailscale-1.68.1-1.fc40) has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2024-81017eae46
FEDORA-2024-bf7e4cdcdc (tailscale-1.58.2-1.fc39) has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2024-bf7e4cdcdc
FEDORA-EPEL-2024-2c8a422454 (tailscale-1.58.2-1.el9) has been submitted as an update to Fedora EPEL 9. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-2c8a422454
FEDORA-2024-bf7e4cdcdc has been pushed to the Fedora 39 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-bf7e4cdcdc` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-bf7e4cdcdc See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2024-2c8a422454 has been pushed to the Fedora EPEL 9 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-2c8a422454 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-81017eae46 has been pushed to the Fedora 40 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-81017eae46` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-81017eae46 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-f42b8bc8e4 has been pushed to the Fedora 40 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-f42b8bc8e4` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-f42b8bc8e4 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-f42b8bc8e4 (tailscale-1.68.2-2.fc40) has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report.