https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #8 from hardt@kit.edu --- 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%%7Bversion%7D/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