https://bugzilla.redhat.com/show_bug.cgi?id=1997994 --- Comment #8 from hardt@xxxxxxx --- Hi There, I'm the debian packager of oidc-agent and a colleague of the developer. I'm replying inline to help getting this package done. I also wrote the initial specfile. Whatever I report as "i have done" refers to a PR in upstream. (In reply to Ben Beasley from comment #7) > Package Review > ============== > > Legend: > [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated > > ===== Issues ===== > > - Instead of > > License: MIT > > you will need something like > > # The entire source is MIT except: > # - src/oidc-prompt/mustache/ is ISC; it is used in oidc-prompt, which > is > # in the -desktop subpackage We are dropping the oidc-desktop package for now (since core functionality is in oidc-agent-cli) > # - src/oidc-gen/qr.c is GPLv2+; it is ussed in oidc-gen, which is in > the > # -cli package > License: MIT I'm reflecting this in a comment in the spec file. I've updated Licenses for all the subpackages > and then > > %package cli > […] > License: MIT and GPLv2+ > > […] > > %package desktop > […] > License: MIT and ISC > > - The ISC license requires that a copy of its text must be included. See > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#_license_text > for options. Or figure out how to unbundle mustach. > > - Even though they are not installed, you should remove pre-compiled Windows > binaries in %prep: > > # Remove pre-compiled Windows binaries > find . -type f \( -name '*.exe' -o -name '*.dll' \) -print -delete We have a make target `rpmsource` that excludes these files, by excluding the whole `windows` folder (at docker, .git, gitbook) > - There is nothing that owns /etc/X11/Xsession.d in the distribution, but the > “desktop” subpackage installs files there. > > First, verify that the file you install there is actually doing something > useful. Fixed by removing oidc-agent-desktop > Then, consider which of the cases in > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_file_and_directory_ownership > best applies. Maybe this package needs to co-own the directory, or maybe > there is some other package that should be a dependency and should get a PR > to create and own it. > > - Nobody owns /etc/oidc-agent. To %files cli, add: > > %dir %{_sysconfdir}/oidc-agent Done in branch address-rh-bugzilla-1997994 > - The use of, e.g., > > %defattr(-,root,root,-) > > is unnecessary and discouraged. Is this a SUSE-ism? See: We're also building for SUSE. I'm removing them for now. We can re-add them with conditionals, in case SUSE needs them. => Done in branch address-rh-bugzilla-1997994 > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions > > - src/oidc-prompt/html/static/css/lib/bootstrap.min.css is Bootswatch > https://github.com/thomaspark/bootswatch. According to > https://docs.fedoraproject.org/en-US/packaging-guidelines/Web_Assets/#_css, > it cannot be included unless you compile it from the SCSS sources in the > package build. We've addressed this for debian, too. Essentially the packaged version is too old for our requirements (5.1.3), which is not available on most distributions. > - There are several bundled libraries: > > - lib/cJSON/ is cJSON, https://github.com/DaveGamble/cJSON, > https://src.fedoraproject.org/rpms/cjson/ > - lib/list/ is clibs/list, https://github.com/clibs/list > - src/oidc-prompt/mustache is https://gitlab.com/jobol/mustach > > Each must be handled according to > https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling: you > must > unbundle them or satisfy certain requirements for bundling. > > The Makefile variables USE_CJSON_SO and USE_LIST_SO may be relevant. I can set USE_CJSON_SO to 1 (to use the system-installed version, provided by `cjson-devel`) for list I didn't find a package For mustache, developer checked, and claims the existing packages are not what he needs. > - For the patch oidc-agent-desktop-terminal.patch, you should add a comment > about the upstream status, such as a link to an upstream bug tracker or an > explanation of why the patch must be downstream-only. > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_all_patches_should_have_an_upstream_bug_link_or_comment Good point, I'm unaware of this patch. > - You should not use the BuildRoot tag in the spec file: > > BuildRoot: %{_tmppath}/%{name} Ok; Done > - The install rules in the Makefile don’t appear to preserve timestamps. I > believe that > > sed -r -i 's/\b(install )([-\$])/\1-p \2/' Makefile > in %prep would resolve this; I have tested that the sed invocation does > what > I expect, but I haven’t tested a build with the patched Makefile. This > would > be considered a patch and should be offered upstream > > (https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_patch_guidelines). Thanks; Applied upstream (in address-rh-bugzilla-1997994 branch) > - According to rpmlint: > > oidc-agent-desktop.x86_64: W: position-independent-executable-suggested > /usr/bin/oidc-prompt > > at least oidc-prompt is compiled without PIE. Please investigate. > > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_pie Fixed, but we've removed oidc-agent-desktop > - The base package (oidc-agent) appears to contain only the license and > README.md, and none of the other packages depend on it. The README.md is also > already installed with liboidc-agent4. In my opinion, the existence of this > oidc-agent binary RPM is useless and confusing. I strongly suggest removing > the %files section for the base package entirely so that no oidc-agent > binary RPM is built. We did this, so updates to manually installed oidc-agent packages would still work (even though we've split it into oidc-agent-cli and oidc-agent-desktop). But since you say _strongly_ I take it that meta-packages are not intended to exist here, and I removed the %files section. (Please tell me if there is another way to have such a meta package). The readme went to oidc-agent-cli, which provides the core functionality and is required by all others. > - The package links against libsodium-static, but in general, > > Executables and libraries SHOULD NOT be linked statically against > libraries > which come from other packages. > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_statically_linking_executables > > If you cannot use the libsodium shared library, please add a comment to the > spec file justifying why static linking is required. > ===== Notes (no change required) ===== > > - If you like, you can drop > > %license LICENSE > > from subpackages other than liboidc-agent4, since the others depend on it > (directly or indirectly). > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#subpackage-licensing Ok; done > - Co-ownership of %{_datadir}/bash-completion/completions/ is acceptable, but > this directory is owned by the “filesystem” package, so it would be better > to > just own the files that this package installs there. ok; done > - A better source archive URL would be: > > Source0: > https://github.com/indigo-dc/oidc-agent/archive/v%{version}/oidc-agent- > %{version}.tar.gz > > or > > Source0: %{url}/archive/v%{version}/oidc-agent-%{version}.tar.gz > > That way, the tarball would be easier to identify and its name would match > the name of the directory it contains. Ok; done (but I didn't test it locally) > - I have not attempted to evaluate whether any systemd unit files are > required. No, we're going without > - You can ignore all of the rpmlint diagnostics from debuginfo packages. > > The following are not serious problems: > > liboidc-agent-devel.x86_64: W: summary-not-capitalized oidc-agent > library development files > liboidc-agent4.x86_64: W: summary-not-capitalized oidc-agent library > > although you could consider rewriting as, e.g., “Library for oidc-agent” Done > - In general, I question whether maintaining a single-source spec file for > Fedora and other RPM-based distributions will be a practical endeavor, > since > the packaging guidelines differ so significantly. Note that even if you > choose to do this (and it is possible, with enough conditionals), the > Fedora > spec file will diverge due to: > > - Mass rebuilds (which bump the release and add a changelog entry) > - Occasional mass spec file changes enacted by provenpackagers Right. What would you suggest? All conditionals in the main specfile and then includes to distribution specific ones? > - You should not use %{__rm} and %{__sed}; prefer rm and sed instead. I'm not aware of using those. > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros > > ===== MUST items ===== [..] omitting checked ones. > [!]: License field in the package spec file matches the actual license. > Note: Checking patched sources after %prep for licenses. Licenses > found: "Unknown or generated", "MIT License", "*No copyright* MIT > License", "GNU Lesser General Public License v2.1 or later", "ISC > License", "*No copyright* [generated file]", "MIT License [generated > file]", "*No copyright* ISC License". 535 files have unknown license. > Detailed output of licensecheck in /home/reviewer/oidc-agent/review- > oidc-agent/licensecheck.txt > > See note in Issues should be fixed > [!]: Package requires other packages for directories it uses. > Note: No known owner of /etc/X11/Xsession.d, /etc/oidc-agent Removed the entire -desktop package, as the -cli subpackage provides the crucial functionality > [!]: Package must own all directories that it creates. > Note: Directories without known owners: /etc/X11/Xsession.d, > /etc/oidc-agent Fixed > [-]: Package does not own files or directories owned by other packages. Fixed (was /etc/bash-completion/... > [!]: Package contains no bundled libraries without FPC exception. > [!]: Each %files section contains %defattr if rpm < 4.4 > Note: %defattr present but not needed Fixed > [-]: If the package is a rename of another package, proper Obsoletes and > Provides are present. oidc-agent-cli provides oidc-agent > [?]: Package contains systemd file(s) if in need. none needed > [-]: Large documentation must go in a -doc subpackage. Large could be size > (~1MB) or number of files. > Note: Documentation size is 20480 bytes in 2 files. not applicable > ===== SHOULD items ===== > > Generic: > [!]: Buildroot is not present > Note: Invalid buildroot found: %{_tmppath}/%{name} > See: https://docs.fedoraproject.org/en-US/packaging-guidelines/ Fixed > [!]: If the source package does not include license text(s) as a separate > file from upstream, the packager SHOULD query upstream to include it. > > There is a license file for the MIT license, but you must provide one > for > mustach’s ISC license if you cannot unbundle it. Can't unbundle, license file fixed > [!]: Patches link to upstream bugs/comments/lists or are otherwise > justified. can't reproduce > [-]: Sources are verified with gpgverify first in %prep if upstream > publishes signatures. > Note: gpgverify is not used. > > [!]: Packages should try to preserve timestamps of original installed > files. Fixed -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=1997994 _______________________________________________ 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 on the list, report it: https://pagure.io/fedora-infrastructure
