[Bug 1997994] New: Review Request: oidc-agent - CLI tools for managing OIDC access tokens
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Bug ID: 1997994 Summary: Review Request: oidc-agent - CLI tools for managing OIDC access tokens Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: vokac@fjfi.cvut.cz QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://vokac.fedorapeople.org/oidc-agent.spec SRPM URL: https://vokac.fedorapeople.org/oidc-agent-4.1.1-1.el7.src.rpm Description: Set of tools to manage OpenID Connect tokens and make them easily usable from the command line. These tools follow ssh-agent design, so OIDC tokens can be handled in a similar way as ssh keys. Fedora Account System Username: vokac
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Petr Vokac vokac@fjfi.cvut.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value
--- Comment #1 from Petr Vokac vokac@fjfi.cvut.cz --- Upstream developers decided to split files in several independent packages, following their decision I updated spec & src.rpm files
https://vokac.fedorapeople.org/oidc-agent.spec-4.1.1-3 https://vokac.fedorapeople.org/oidc-agent-4.1.1-3.el7.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Hirotaka Wakabayashi hiwkby@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hiwkby@yahoo.com
--- Comment #2 from Hirotaka Wakabayashi hiwkby@yahoo.com --- Hello Petr, I think "BuildRequires: gcc" is required to build oidc-agent correctly.
FYI: Here is a part of build error message in Koji. ``` + make make: gcc: No such file or directory make: *** [Makefile:242: obj/oidc-agent/agent_state.o] Error 127 ``` https://kojipkgs.fedoraproject.org//work/tasks/633/75760633/build.log
Best, Hirotaka Wakabayashi
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |code@musicinmybrain.net
--- Comment #3 from Ben Beasley code@musicinmybrain.net --- A few comments, based on a quick look at the spec file. I didn’t try to build it or run fedora-review.
----
Runtime dependencies due to shared library linking are automatically generated. You should not need lines like:
Requires: libsodium >= 1.0.18
You might still need those that provide command-line tools or other resources.
---- Versioned dependencies are discouraged when the previous three Fedora releases would satisfy the dependency, i.e.:
BuildRequires: libcurl-devel >= 7.29
should be
BuildRequires: libcurl-devel
after consulting https://src.fedoraproject.org/rpms/curl. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_package_dependen....
----
You can more cleanly write
%package -n oidc-agent-cli
as
%package cli
and so on.
----
You should use the %make_build and %make_install macros.
----
You should remove
%defattr(-,root,root,-)
per https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions.
----
You shouldn’t package static libraries without a strong justification:
%{_libdir}/liboidc-agent.a
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_packaging_static...
----
You mustn’t assume gzip compression for man pages (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages).
----
Do you have a reason for using %config instead of %config(noreplace)?
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_configuration_fi...
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Ben Cotton bcotton@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bcotton@redhat.com Flags| |needinfo?(vokac@fjfi.cvut.c | |z)
--- Comment #4 from Ben Cotton bcotton@redhat.com --- Petr, are you still interested in adding this package? If so, I'll take the review. If not, we can close this request.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Petr Vokac vokac@fjfi.cvut.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(vokac@fjfi.cvut.c | |z) |
--- Comment #5 from Petr Vokac vokac@fjfi.cvut.cz --- I'll try to fix reported issues this week.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #6 from Petr Vokac vokac@fjfi.cvut.cz --- Sorry for such a big delay, I finally went through documentation and notes in comment #3. Updated packages
https://vokac.fedorapeople.org/oidc-agent-4.3.2-1.el7.src.rpm https://vokac.fedorapeople.org/oidc-agent.spec-4.3.2-1
With installed packages on CentOS7 rpmlint gives me this output
===== oidc-agent ===== oidc-agent.x86_64: E: no-binary 1 packages and 0 specfiles checked; 1 errors, 0 warnings.
Main package could be "noarch" but then it is not possible to have arch dependend subpackages. This package just install CLI tools as a dependency, historically this package contained everything from oidc-agent-cli+oidc-agent-desktop but they were spitted not to install GUI dependencies by default.
===== oidc-agent-cli ===== oidc-agent-cli.x86_64: W: spelling-error %description -l en_US logins -> losing, login, loins 1 packages and 0 specfiles checked; 0 errors, 1 warnings. ===== oidc-agent-desktop ===== 1 packages and 0 specfiles checked; 0 errors, 0 warnings. ===== liboidc-agent4 ===== liboidc-agent4.x86_64: W: spelling-error Summary(en_US) oidc -> oi dc, oi-dc, ovoid liboidc-agent4.x86_64: W: summary-not-capitalized C oidc-agent library liboidc-agent4.x86_64: W: spelling-error %description -l en_US oidc -> oi dc, oi-dc, ovoid liboidc-agent4.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 4 warnings.
This package naming scheme comes from original debian packages and it should allow in future to make new liboidc-agent5 release with incompatible changes while keeping support for old software linked with current liboidc-agent4.
===== liboidc-agent-devel ===== liboidc-agent-devel.x86_64: W: no-dependency-on liboidc-agent/liboidc-agent-libs/libliboidc-agent liboidc-agent-devel.x86_64: W: spelling-error Summary(en_US) oidc -> oi dc, oi-dc, ovoid liboidc-agent-devel.x86_64: W: summary-not-capitalized C oidc-agent library development files liboidc-agent-devel.x86_64: W: spelling-error %description -l en_US oidc -> oi dc, oi-dc, ovoid liboidc-agent-devel.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 5 warnings.
For development we would like to support just latest oidc-agent libraries.
Same oidc-agent.spec file is also used for OpenSuse build and I was trying not to diverge and preferably keep same RPM spec file for all distributions.
Petr
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |code@musicinmybrain.net
--- Comment #7 from Ben Beasley code@musicinmybrain.net --- 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 # - src/oidc-gen/qr.c is GPLv2+; it is ussed in oidc-gen, which is in the # -cli package License: MIT
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/LicensingGuideline... 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
- 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.
Then, consider which of the cases in
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directo... 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
- The use of, e.g.,
%defattr(-,root,root,-)
is unnecessary and discouraged. Is this a SUSE-ism? See:
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.
- 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.
- 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_shou...
- You should not use the BuildRoot tag in the spec file:
BuildRoot: %{_tmppath}/%{name}
- 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).
- 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
- 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.
- 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_linki...
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/LicensingGuideline...
- 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.
- A better source archive URL would be:
Source0: https://github.com/indigo-dc/oidc-agent/archive/v%%7Bversion%7D/oidc-agent-%...
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.
- I have not attempted to evaluate whether any systemd unit files are required.
- 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”
- 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
- You should not use %{__rm} and %{__sed}; prefer rm and sed instead.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: ldconfig not called in %post and %postun for Fedora 28 and later. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. [x]: Development (unversioned) .so files in -devel subpackage, if present.
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. [!]: 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
[x]: License file installed when any subpackage combination is installed. [!]: Package requires other packages for directories it uses. Note: No known owner of /etc/X11/Xsession.d, /etc/oidc-agent [!]: Package must own all directories that it creates. Note: Directories without known owners: /etc/X11/Xsession.d, /etc/oidc-agent [-]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/share/bash- completion/completions(tealdeer, poetry, xss-lock, gammu, devscripts- checkbashisms, doctl, python3-catkin_tools, smc-tools, unar, gpaste, mt-st, task, tracker, xca, git-delta, just, minipro, driverctl, maven, coccinelle-bash-completion, gh, git-core, dosbox-staging, mdbtools, hstr, kmod, nnn, lightdm, lxi-tools, glib2, yadifa-tools, flameshot, swaylock, wlogout, pcp, alacritty, environment-modules, cowsay, httpie, source-highlight, timew, tldr, fedora-update-feedback, kompose, packit, rpmdevtools, lmms, awscli, vagrant, skopeo, cobbler, python-django-bash-completion, libnbd-bash-completion, golang-github- tdewolff-minify, repo, composer, python3-tqdm, dnf, beaker-client, filesystem, calibre, osslsigncode, firewalld, playerctl, devscripts, toolbox, mtr, reprepro, monotone, pbuilder, docker-compose, lastpass- cli, libqmi-utils, zeitgeist, libappstream-glib, zypper, datamash, fd- find, pdfgrep, yadifa, bubblewrap, breezy, guestfs-tools-bash- completion, restic, darcs, hyperfine, dconf-editor, docopt, ethtool, dub, tio, switchtec, eg, dotnet-host, nitrokey-app, stress-ng, python3-trezor, ripgrep, GMT-common, swayidle, opensc, python3-streamlink, hashcat, libmbim-utils, nbdkit-bash-completion, exa, firejail, cpu-x, calf, licensecheck, falkon, chocolate-doom, fedpkg, lxc, chatty, gopass, buildah, stratis-cli, rubygem-ronn-ng, clevis, python3-pip, hcloud, sway, ffsend, exercism, ModemManager, bash-completion, rpmspectool, croc, nordugrid-arc-client, tig, vultr- cli, etckeeper, libguestfs-bash-completion, azure-cli, subversion, skim, bodhi-cli, ldc, flatpak, linode-cli, vcsh, zola) [x]: %build honors applicable compiler flags or justifies otherwise. [!]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [!]: Each %files section contains %defattr if rpm < 4.4 Note: %defattr present but not needed [x]: 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. [?]: 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 20480 bytes in 2 files. [x]: Package complies to the Packaging Guidelines
(except as noted)
[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]: 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]: %config files are marked noreplace or the reason is justified. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package contains desktop file if it is a GUI application. [x]: Package installs a %{name}.desktop using desktop-file-install or desktop-file-validate if there is such a file. [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]: No %config files under /usr. [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]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [!]: Buildroot is not present Note: Invalid buildroot found: %{_tmppath}/%{name} See: https://docs.fedoraproject.org/en-US/packaging-guidelines/ [!]: 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.
[x]: Final provides and requires are sane (see attachments). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in oidc- agent-cli , liboidc-agent4 , liboidc-agent-devel , oidc-agent-desktop
Appropriate fully-versioned dependencies appear to be present.
[?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass.
Upstream provides no tests, but the mandatory desktop-file-validate invocation is present.
[!]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [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]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on debuginfo package(s). Note: There are rpmlint messages (see attachment). [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.
Rpmlint ------- Cannot parse rpmlint output:
Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output:
Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output:
Source checksums ---------------- https://github.com/indigo-dc/oidc-agent/archive/refs/tags/v4.3.2.tar.gz : CHECKSUM(SHA256) this package : 8f18afa7a066e7a781d3c239afd183115205edacfd58806906c9f48f24bfe2b0 CHECKSUM(SHA256) upstream package : 8f18afa7a066e7a781d3c239afd183115205edacfd58806906c9f48f24bfe2b0
Requires -------- oidc-agent (rpmlib, GLIBC filtered): oidc-agent-desktop(x86-64)
oidc-agent-cli (rpmlib, GLIBC filtered): /usr/bin/bash config(oidc-agent-cli) jq libc.so.6()(64bit) libcurl.so.4()(64bit) libglib-2.0.so.0()(64bit) libmicrohttpd.so.12()(64bit) liboidc-agent4(x86-64) libqrencode.so.4()(64bit) libsecret-1.so.0()(64bit) libsodium.so.23()(64bit) rtld(GNU_HASH)
liboidc-agent4 (rpmlib, GLIBC filtered): libc.so.6()(64bit) libsodium.so.23()(64bit) rtld(GNU_HASH)
liboidc-agent-devel (rpmlib, GLIBC filtered): liboidc-agent.so.4()(64bit) liboidc-agent4(x86-64)
oidc-agent-desktop (rpmlib, GLIBC filtered): config(oidc-agent-desktop) libatk-1.0.so.0()(64bit) libc.so.6()(64bit) libcairo-gobject.so.2()(64bit) libcairo.so.2()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgdk-3.so.0()(64bit) libgdk_pixbuf-2.0.so.0()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgmodule-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libgtk-3.so.0()(64bit) libharfbuzz.so.0()(64bit) libjavascriptcoregtk-4.0.so.18()(64bit) libm.so.6()(64bit) libpango-1.0.so.0()(64bit) libpangocairo-1.0.so.0()(64bit) libsodium.so.23()(64bit) libsoup-2.4.so.1()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libwebkit2gtk-4.0.so.37()(64bit) libz.so.1()(64bit) oidc-agent-cli(x86-64) rtld(GNU_HASH) webkitgtk4 xterm
oidc-agent-debuginfo (rpmlib, GLIBC filtered):
oidc-agent-debugsource (rpmlib, GLIBC filtered):
Provides -------- oidc-agent: oidc-agent oidc-agent(x86-64)
oidc-agent-cli: config(oidc-agent-cli) oidc-agent-cli oidc-agent-cli(x86-64)
liboidc-agent4: liboidc-agent.so.4()(64bit) liboidc-agent4 liboidc-agent4(x86-64)
liboidc-agent-devel: liboidc-agent-devel liboidc-agent-devel(x86-64)
oidc-agent-desktop: application() application(oidc-gen.desktop) config(oidc-agent-desktop) mimehandler(x-scheme-handler/edu.kit.data.oidc-agent) oidc-agent-desktop oidc-agent-desktop(x86-64)
oidc-agent-debuginfo: oidc-agent-debuginfo oidc-agent-debuginfo(x86-64)
oidc-agent-debugsource: oidc-agent-debugsource oidc-agent-debugsource(x86-64)
Generated by fedora-review 0.8.0 (e988316) last change: 2022-04-07 Command line :/usr/bin/fedora-review -n oidc-agent Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, Generic, C/C++ Disabled plugins: Perl, R, Haskell, SugarActivity, PHP, Ocaml, fonts, Python, Java Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH ============================ rpmlint session starts ============================ rpmlint: 2.2.0 configuration: /usr/lib/python3.10/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 11
liboidc-agent4-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/lib64/liboidc-agent.so.4.3.2-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/oidc-add-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/oidc-agent-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/oidc-gen-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/oidc-token-4.3.2-1.fc37.x86_64.debug oidc-agent-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/.dwz/oidc-agent-4.3.2-1.fc37.x86_64 oidc-agent-desktop-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/oidc-prompt-4.3.2-1.fc37.x86_64.debug 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 oidc-agent-debuginfo.x86_64: E: statically-linked-binary /usr/lib/debug/.dwz/oidc-agent-4.3.2-1.fc37.x86_64 oidc-agent-desktop-debuginfo.x86_64: E: statically-linked-binary /usr/lib/debug/usr/bin/oidc-prompt-4.3.2-1.fc37.x86_64.debug liboidc-agent4-debuginfo.x86_64: E: shared-library-without-dependency-information /usr/lib/debug/usr/lib64/liboidc-agent.so.4.3.2-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: E: shared-library-without-dependency-information /usr/lib/debug/usr/bin/oidc-add-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: E: shared-library-without-dependency-information /usr/lib/debug/usr/bin/oidc-agent-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: E: shared-library-without-dependency-information /usr/lib/debug/usr/bin/oidc-gen-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: E: shared-library-without-dependency-information /usr/lib/debug/usr/bin/oidc-token-4.3.2-1.fc37.x86_64.debug oidc-agent-desktop.x86_64: W: position-independent-executable-suggested /usr/bin/oidc-prompt oidc-agent-desktop-debuginfo.x86_64: W: position-independent-executable-suggested /usr/lib/debug/usr/bin/oidc-prompt-4.3.2-1.fc37.x86_64.debug oidc-agent-cli-debuginfo.x86_64: W: no-documentation oidc-agent-debuginfo.x86_64: W: no-documentation oidc-agent-debugsource.x86_64: W: no-documentation oidc-agent-desktop-debuginfo.x86_64: W: no-documentation oidc-agent.x86_64: E: no-binary oidc-agent-debuginfo.x86_64: E: missing-PT_GNU_STACK-section /usr/lib/debug/.dwz/oidc-agent-4.3.2-1.fc37.x86_64 oidc-agent-debuginfo.x86_64: W: hidden-file-or-dir /usr/lib/debug/.dwz oidc-agent-debuginfo.x86_64: W: hidden-file-or-dir /usr/lib/debug/.dwz oidc-agent-desktop.x86_64: W: desktopfile-without-binary /usr/share/applications/oidc-gen.desktop bash liboidc-agent4-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/ab/337ed01bb703c25d94618e56a57a6447ca87b0 ../../../.build-id/ab/337ed01bb703c25d94618e56a57a6447ca87b0 oidc-agent-cli-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/2b/7fd3e2970f25f510d39195c245ef858e6135cc ../../../.build-id/2b/7fd3e2970f25f510d39195c245ef858e6135cc oidc-agent-cli-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/5e/4bbe2d05219a458be2307bbc04c2d25243cd32 ../../../.build-id/5e/4bbe2d05219a458be2307bbc04c2d25243cd32 oidc-agent-cli-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/c7/3dd8d28adc87e6bdd2cf0214908398ec375a38 ../../../.build-id/c7/3dd8d28adc87e6bdd2cf0214908398ec375a38 oidc-agent-cli-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/ec/78b3cc779cadae8899637f962dbdd3042f8163 ../../../.build-id/ec/78b3cc779cadae8899637f962dbdd3042f8163 oidc-agent-desktop-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/84/3c93196a30a0e0dfde2d3155777cf95a1d9e77 ../../../.build-id/84/3c93196a30a0e0dfde2d3155777cf95a1d9e77 11 packages and 0 specfiles checked; 9 errors, 24 warnings, 9 badness; has taken 1.5 s
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review?
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 inthe # -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.gzThat 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 onefor 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
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #9 from hardt@kit.edu --- I also removed the dependency on libsodium-static
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |decathorpe@gmail.com
--- Comment #10 from Fabio Valentini decathorpe@gmail.com --- (In reply to hardt from comment #8)
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.
Just two quick notes:
1) Using conditionals for distros other than Fedora or EPEL is forbidden in Fedora .spec files: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_legibility
2) The .spec file in Fedora MUST be considered the canonical source. If the upstream project decides to also provide a .spec file (for whatever purposes: test builds, etc.), downstream changes to the .spec file MUST NOT be overwritten when merging changes from the upstream project. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_maintenance...
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #11 from Ben Beasley code@musicinmybrain.net --- (In reply to Fabio Valentini from comment #10)
- Using conditionals for distros other than Fedora or EPEL is forbidden in
Fedora .spec files: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_legibility
Interesting! I knew it was not encouraged, but I had not run across that particular section of the guidelines. Thanks for pointing that out.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #12 from Ben Beasley code@musicinmybrain.net --- Thanks! I’ve skimmed your responses.
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.
Unfortunately, I’m not aware of any way around the ban on pre-compiled CSS. The current guidelines around JavaScript and web assets are very strict—arguably, so strict that modern web assets usually can’t be packaged.
I can set USE_CJSON_SO to 1 (to use the system-installed version, provided by `cjson-devel`)
Sounds good.
for list I didn't find a package
It can be packaged as a dependency.
For mustache, developer checked, and claims the existing packages are not what he needs.
As in, https://src.fedoraproject.org/rpms/mustache can’t be used because it is not actually the same library, or the upstream version of https://gitlab.com/jobol/mustach (not currently packaged in Fedora as far as I can tell) can’t be used, e.g. because the version in oidc-agent is forked?
Bundling is allowed in Fedora, but there are specific conditions that have to be met, and the bundling has to be properly justified and indicated with virtual Provides. For example “nobody has packaged the dependency yet” does not allow you to bundle it, but “upstream doesn’t support building against an external library and I publicly contacted them at https://example.com/link about whether it could be possible in the future” does.
Right. What would you suggest? All conditionals in the main specfile and then includes to distribution specific ones?
Especially considering Fabio’s reminder about non-Fedora and non-EPEL conditionals, I think you’ll just need to commit to the idea that you will need to merge changes into the Fedora spec file, and will not be able to maintain a single source for all distributions.
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).
In this case, I missed that oidc-agent had “Requires: %{name}-desktop%{?_isa} = %{version}-%{release}”, so I thought it was just a useless nearly-empty package rather than a metapackage. Metapackages are common in Fedora. Usually they have an empty %files section, and that seems to make sense here since other subpackages already have the license and readme. Please feel free to use oidc-agent as a metapackage for convenient installation.
If you are ever inclined to use a metapackage strictly for upgrade compatibility, Providing the old name is usually a better choice. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#renaming-or-repla... for an example, but note that you don’t need the full Package Renaming Process to rename a subpackage.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #13 from hardt@kit.edu --- (In reply to Ben Beasley from comment #12)
Thanks! I’ve skimmed your responses.
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.
Unfortunately, I’m not aware of any way around the ban on pre-compiled CSS. The current guidelines around JavaScript and web assets are very strict—arguably, so strict that modern web assets usually can’t be packaged.
Ok, with dropping the -desktop subpackage, we're no longer include bootswatch/bootstrap
It's still being built (since I don't want to touch the Makefile right now), but none of this ends up in any output package. I suppose the right process is to remove the files and patch the Makefile in the %prep step of the fedora specfile, right?
for list I didn't find a package
It can be packaged as a dependency.
Who is supposed to do this? Is there a process, or will I (i.e. upstream) be left with this?
For mustache, developer checked, and claims the existing packages are not what he needs.
As in, https://src.fedoraproject.org/rpms/mustache can’t be used because it is not actually the same library, or the upstream version of https://gitlab.com/jobol/mustach (not currently packaged in Fedora as far as I can tell) can’t be used, e.g. because the version in oidc-agent is forked?
Bundling is allowed in Fedora, but there are specific conditions that have to be met, and the bundling has to be properly justified and indicated with virtual Provides. For example “nobody has packaged the dependency yet” does not allow you to bundle it, but “upstream doesn’t support building against an external library and I publicly contacted them at https://example.com/link about whether it could be possible in the future” does.
Since we don't build the -desktop subpackage any longer, we don't need to address this right now. For the record: we use the mustache version plain from github, so once there is another package providing it, we can revisit the -desktop subpackage.
Right. What would you suggest? All conditionals in the main specfile and then includes to distribution specific ones?
Especially considering Fabio’s reminder about non-Fedora and non-EPEL conditionals, I think you’ll just need to commit to the idea that you will need to merge changes into the Fedora spec file, and will not be able to maintain a single source for all distributions.
I guess this Fedora spec file is kept in a different repository, to which the package maintainer has access? I'm not really sure how my interface to this is.
I was building here [1] using our upstream specfile [2]
Copr is als the reason why I don't understand how to use different specfiles for different distributions.
[1] https://copr.fedorainfracloud.org/coprs/marcvs/oidc-agent/build/4686340/ [2] https://github.com/indigo-dc/oidc-agent/blob/address-rh-bugzilla-1997994/rpm...
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).
In this case, I missed that oidc-agent had “Requires: %{name}-desktop%{?_isa} = %{version}-%{release}”, so I thought it was just a useless nearly-empty package rather than a metapackage. Metapackages are common in Fedora. Usually they have an empty %files section, and that seems to make sense here since other subpackages already have the license and readme. Please feel free to use oidc-agent as a metapackage for convenient installation.
If you are ever inclined to use a metapackage strictly for upgrade compatibility, Providing the old name is usually a better choice. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#renaming-or- replacing-existing-packages for an example, but note that you don’t need the full Package Renaming Process to rename a subpackage.
Ok; I've added it back in (but it does no longer depend on the -desktop subpackage (obviously)
Using [1] and [2], I'm left with three rpmlint warnings that I didn't manage to fix:
- oidc-agent.spec: W: invalid-url Source0: oidc-agent-4.3.2.tar.gz => I didn't ever specify that URL - liboidc-agent-devel.x86_64: W: dangling-relative-symlink /usr/lib/.build-id/44/d338004058c396a41f2c9b4615f269a1a6c0a4 ../../../../usr/bin/oidc-prompt => .build-id is not mentioned in any %files directive. %excluding it didn't help either. I hope this is just an artifact of my local build docker. - oidc-agent-cli.x86_64: W: invalid-license LGPL-2.1+ => This license is specified by the author/owner of src/oidc-gen/qr.c
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #14 from Fabio Valentini decathorpe@gmail.com ---
I guess this Fedora spec file is kept in a different repository, to which the package maintainer has access? I'm not really sure how my interface to this is.
Yes, upon approval, every Fedora package gets an individual git repository created for it, with branches for different releases of Fedora; like this "rawhide" branch in the dist-git repo for "clang": https://src.fedoraproject.org/rpms/clang/tree/rawhide
This git repository is the only source from which packages can be built. And it is supposed to be the place where the .spec file is developed.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #15 from Ben Beasley code@musicinmybrain.net --- Are you already a Fedora packager, or are you in search of a sponsor?
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #16 from Ben Beasley code@musicinmybrain.net --- (I guess that question was directed at the original reporter, Petr Vocak, since they would be the one requesting the repository.)
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #17 from hardt@kit.edu --- I think Petr is the packager, originally.
If Petr does not object, and if it's not adding much more overhead, I'd be fine to become the fedora packager for oidc-agent
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #18 from Petr Vokac vokac@fjfi.cvut.cz --- Yes, I agree it would be better if this package is maintained by Marcus Hardt, because he makes these packages for other distribution and he is directly in touch with developers.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #19 from Fabio Valentini decathorpe@gmail.com --- Just be aware that whoever ends up maintaining the package also needs the person who filed the review request, as that is checked before package creation.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #20 from Ben Beasley code@musicinmybrain.net --- (In reply to hardt from comment #13)
for list I didn't find a package
It can be packaged as a dependency.
Who is supposed to do this? Is there a process, or will I (i.e. upstream) be left with this?
It’s up to a Fedora package’s maintainer (or prospective maintainer, for a new package) to make sure all dependencies are packaged. So unless you can find somebody else who also cares about this dependency, it is up to you. It’s a pretty straightforward package.
That said, in this case I’m willing to do the initial packaging. I’m working with upstream a bit (https://github.com/clibs/list/pull/36, https://github.com/clibs/list/pull/39, https://github.com/clibs/list/issues/38) before submitting a review request. Ideally the maintainers of the oidc-agent package will co-maintain the resulting package.
Since we don't build the -desktop subpackage any longer, we don't need to address this right now. For the record: we use the mustache version plain from github, so once there is another package providing it, we can revisit the -desktop subpackage.
Yup, as for clibs/list, if you need a dependency that is not packaged but can be packaged, you must either package it yourself or find somebody else you can convince to do it.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #21 from hardt@kit.edu --- I tried to package clibs/list, using this specfile[1] leading to this result[2]
The problem is: error: Empty %files file /builddir/build/BUILD/list-master/debugsourcefiles.list Empty %files file /builddir/build/BUILD/list-master/debugsourcefiles.list
I've added a patch to add '-g' to the CFLAGS, but I think this is not necessary, as I saw a '-g' parameter to gcc before. Also, it does not change the error message.
[1] https://git.scc.kit.edu/m-team/clibs-packaging/-/tree/main/list [2] https://copr.fedorainfracloud.org/coprs/marcvs/clibs-list/build/4687891/
Regarding becoming a maintainer, is there some process that I should follow?
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #22 from Ben Beasley code@musicinmybrain.net --- (In reply to hardt from comment #21)
I tried to package clibs/list, using this specfile[1] leading to this result[2]
The problem is: error: Empty %files file /builddir/build/BUILD/list-master/debugsourcefiles.list Empty %files file /builddir/build/BUILD/list-master/debugsourcefiles.list
I've added a patch to add '-g' to the CFLAGS, but I think this is not necessary, as I saw a '-g' parameter to gcc before. Also, it does not change the error message.
I have a reasonable spec file ready for it, but I’m waiting to see what upstream says about https://github.com/clibs/list/issues/38 before submitting it for review. I’ll attach a copy to this bug.
Regarding becoming a maintainer, is there some process that I should follow?
See https://docs.fedoraproject.org/en-US/package-maintainers/Joining_the_Package.... Note that I’m a packager sponsor, and would probably be willing to sponsor you once this review is complete.
Even if you want to be the primary maintainer of this package, since Petr Vocak submitted this review request, Petr is the only one who could request the dist-git repository after a successful review. If Petr is not already in the packager group, they would need to follow the same process to join it, although the review can be completed before the requestor is sponsored. Once the repository was created, Petr could then add you as a co-maintainer or even give the package to you as the new primary maintainer.
Alternatively, and only if Petr explicitly agrees, you could submit a new review request and mark this one as a duplicate. Then you would be the initial primary maintainer who could request the dist-git repository.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #23 from Ben Beasley code@musicinmybrain.net --- Created attachment 1899732 --> https://bugzilla.redhat.com/attachment.cgi?id=1899732&action=edit Preliminary spec file for clibs-list dependency
Waiting on upstream feedback in https://github.com/clibs/list/issues/38 before submitting a review request.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #24 from Petr Vokac vokac@fjfi.cvut.cz ---
Even if you want to be the primary maintainer of this package, since Petr Vocak submitted this review request, Petr is the only one who could request the dist-git repository after a successful review. If Petr is not already in ...
I'm already in packager group.
It seems to me easiest if we follow discussion here and I'll assign Marcus access to this package later.
(btw: I'll not be reachable August 3-8)
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #25 from hardt@kit.edu --- Ok; I've setup the accounts according to https://docs.fedoraproject.org/en-US/package-maintainers/Joining_the_Package... Username is marcvs
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(code@musicinmybra | |in.net)
--- Comment #26 from Ben Beasley code@musicinmybrain.net --- Setting NEEDINFO on myself as a reminder to submit clibs/list for review soon. I was really hoping upstream would respond to my query about generic naming.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #27 from Ben Beasley code@musicinmybrain.net --- https://github.com/clibs/list/pull/40#issuecomment-1230234757
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Ben Beasley code@musicinmybrain.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |2123950
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2123950 [Bug 2123950] Review Request: clibs-list - C doubly linked list implementation
https://bugzilla.redhat.com/show_bug.cgi?id=1997994 Bug 1997994 depends on bug 2123950, which changed state.
Bug 2123950 Summary: Review Request: clibs-list - C doubly linked list implementation https://bugzilla.redhat.com/show_bug.cgi?id=2123950
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |CLOSED Resolution|--- |ERRATA
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #29 from hardt@kit.edu --- Hi There,
I found the time to package the dependency clibs/list. It now builds on most distributions on copr. - spec file: https://git.scc.kit.edu/m-team/clibs-packaging/-/raw/main/list/beasley.spec - copr build: https://copr.fedorainfracloud.org/coprs/marcvs/clibs-list/build/5212080/
Two more dependencies are reuquired by oidc-agent 4.4.0 and newer. Both don't build on all distributions, mostly due to missing dependencies: - mustach - spec file: https://gitlab.com/Yestheone/mustach/-/raw/rpm-packaging/packaging/rpm/musta... - copr build: https://copr.fedorainfracloud.org/coprs/marcvs/mustach/build/5211198/ - libjs-bootswatch - spec file: https://git.scc.kit.edu/m-team/upstream/libjs-bootswatch/-/raw/rpm/devel/rpm... - copr build: https://copr.fedorainfracloud.org/coprs/marcvs/libjs-bootswatch/build/520896...
What are the next steps?
Best, Marcus
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #30 from Ben Beasley code@musicinmybrain.net --- Hmm, I guess I failed to follow up here after https://bugzilla.redhat.com/show_bug.cgi?id=1997994#c26 when my clibs-list package was reviewed and I imported it into Fedora. See https://src.fedoraproject.org/rpms/clibs-list/. I’m happy to add you as a co-maintainer to that package.
The other dependencies would have to be submitted for review separately.
Looking at [1] and at the libjs-bootswatch spec file, you are going to have to find a way to compile the CSS from SCSS sources during the RPM build. There are three SCSS compilers in Fedora at the moment: php-scssphp, python3-scss, and rubygem-sass. Maybe one of them will work, so you don’t have to use grunt like upstream does. Fortunately, there doesn’t appear to be any JavaScript[2] in the package.
[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/Web_Assets/#_css [2] https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #31 from Ben Beasley code@musicinmybrain.net --- (In reply to Ben Beasley from comment #30)
Looking at [1] and at the libjs-bootswatch spec file, …
I found https://src.fedoraproject.org/rpms/python-XStatic-bootswatch already in the distribution. It looks like it probably already packages the CSS files you need.
I’m not expressing an opinion on whether it correctly deals with the guidelines issues I noted above; I am guessing it might not, but I didn’t read the spec file closely.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #32 from hardt@kit.edu --- Thanks for pointing me to the JavaScript Docs.
Many distributions do not provide bootswatch themes newer than 3.3.x.
I have not fully understood the reason, except that the openstack web dashboard depends on the older version.
oidc-agent required version 5.
I would append the major version to the package name (libjs-bootswatch5), and adjust path (/usr/share/javascripts/bootswatch5) so that both packages can coexist.
I'll file a review for mustach soon.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #33 from Steve Traylen steve.traylen@cern.ch --- Just looked into packaging this for Fedora and then found this - good.
Are these still the review candidates for Fedora:
https://vokac.fedorapeople.org/oidc-agent.spec-4.1.1-3 https://vokac.fedorapeople.org/oidc-agent-4.1.1-3.el7.src.rpm ?
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review? Assignee|nobody@fedoraproject.org |steve.traylen@cern.ch
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #34 from Steve Traylen steve.traylen@cern.ch --- Sorry no its these:
https://vokac.fedorapeople.org/oidc-agent-4.3.2-1.el7.src.rpm https://vokac.fedorapeople.org/oidc-agent.spec-4.3.2-1
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
--- Comment #35 from Steve Traylen steve.traylen@cern.ch --- [fedora-review-service-build]
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Jakub Kadlčík jkadlcik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/indigo-d | |c/oidc-agent
--- Comment #36 from Jakub Kadlčík jkadlcik@redhat.com --- Copr build: https://copr.fedorainfracloud.org/coprs/build/5701014 (failed)
Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please make sure the package builds successfully at least for Fedora Rawhide.
- If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field
--- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Whiteboard| |NotReady
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
https://bugzilla.redhat.com/show_bug.cgi?id=1997994
Petr Vokac vokac@fjfi.cvut.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |DUPLICATE Last Closed| |2023-03-30 04:09:36
--- Comment #37 from Petr Vokac vokac@fjfi.cvut.cz --- I can't really find time to finish this package so we discussed with Steve Traylen that he'll take over this process in
https://bugzilla.redhat.com/show_bug.cgi?id=2182905
*** This bug has been marked as a duplicate of bug 2182905 ***
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com