https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #8 from hardt(a)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
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.
[..] 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