Bug 1997994 - Review Request: oidc-agent - CLI tools for managing OIDC access tokens
Summary: Review Request: oidc-agent - CLI tools for managing OIDC access tokens
Keywords:
Status: CLOSED DUPLICATE of bug 2182905
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/indigo-dc/oidc-agent
Whiteboard: NotReady
Depends On: 2123950
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-08-26 08:46 UTC by Petr Vokac
Modified: 2023-03-30 04:09 UTC (History)
7 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-03-30 04:09:36 UTC
Type: ---
Embargoed:
steve.traylen: fedora-review?


Attachments (Terms of Use)
Preliminary spec file for clibs-list dependency (2.17 KB, text/plain)
2022-07-27 16:44 UTC, Ben Beasley
no flags Details

Description Petr Vokac 2021-08-26 08:46:00 UTC
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

Comment 1 Petr Vokac 2021-09-03 09:53:31 UTC
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

Comment 2 Hirotaka Wakabayashi 2021-09-16 03:22:44 UTC
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

Comment 3 Ben Beasley 2021-09-16 14:35:58 UTC
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_dependencies.

----

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_libraries

----

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_files

Comment 4 Ben Cotton 2022-01-14 15:16:55 UTC
Petr, are you still interested in adding this package? If so, I'll take the review. If not, we can close this request.

Comment 5 Petr Vokac 2022-01-17 09:31:25 UTC
I'll try to fix reported issues this week.

Comment 6 Petr Vokac 2022-07-23 09:27:37 UTC
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

Comment 7 Ben Beasley 2022-07-24 14:15:47 UTC
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/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

- 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_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

- 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_should_have_an_upstream_bug_link_or_comment

- 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_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

- 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%{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.

- 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

Comment 8 hardt 2022-07-25 15:54:12 UTC
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

Comment 9 hardt 2022-07-25 15:55:11 UTC
I also removed the dependency on libsodium-static

Comment 10 Fabio Valentini 2022-07-25 16:12:03 UTC
(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_and_canonicity

Comment 11 Ben Beasley 2022-07-25 16:15:33 UTC
(In reply to Fabio Valentini from comment #10)
> 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

Interesting! I knew it was not encouraged, but I had not run across that particular section of the guidelines. Thanks for pointing that out.

Comment 12 Ben Beasley 2022-07-25 16:30:27 UTC
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-replacing-existing-packages for an example, but note that you don’t need the full Package Renaming Process to rename a subpackage.

Comment 13 hardt 2022-07-26 15:21:29 UTC
(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/oidc-agent.spec


> > 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

Comment 14 Fabio Valentini 2022-07-26 15:50:20 UTC
> 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.

Comment 15 Ben Beasley 2022-07-26 15:53:12 UTC
Are you already a Fedora packager, or are you in search of a sponsor?

Comment 16 Ben Beasley 2022-07-26 15:54:52 UTC
(I guess that question was directed at the original reporter, Petr Vocak, since they would be the one requesting the repository.)

Comment 17 hardt 2022-07-26 15:59:00 UTC
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

Comment 18 Petr Vokac 2022-07-26 16:31:07 UTC
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.

Comment 19 Fabio Valentini 2022-07-26 16:45:03 UTC
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.

Comment 20 Ben Beasley 2022-07-26 21:29:03 UTC
(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.

Comment 21 hardt 2022-07-27 14:13:20 UTC
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?

Comment 22 Ben Beasley 2022-07-27 16:42:32 UTC
(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_Maintainers/. 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.

Comment 23 Ben Beasley 2022-07-27 16:44:01 UTC
Created attachment 1899732 [details]
Preliminary spec file for clibs-list dependency

Waiting on upstream feedback in https://github.com/clibs/list/issues/38 before submitting a review request.

Comment 24 Petr Vokac 2022-07-28 08:39:20 UTC
> 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)

Comment 25 hardt 2022-08-01 08:26:41 UTC
Ok; I've setup the accounts according to https://docs.fedoraproject.org/en-US/package-maintainers/Joining_the_Package_Maintainers/
Username is marcvs

Comment 26 Ben Beasley 2022-08-01 12:46:45 UTC
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.

Comment 28 Package Review 2022-09-01 00:45:24 UTC
This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Comment 29 hardt 2023-01-10 13:06:58 UTC
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/mustach.spec?inline=false
    - 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/libjs-bootswatch.spec
    - copr build: https://copr.fedorainfracloud.org/coprs/marcvs/libjs-bootswatch/build/5208963/

What are the next steps?

Best,
Marcus

Comment 30 Ben Beasley 2023-01-10 14:47:40 UTC
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/

Comment 31 Ben Beasley 2023-01-10 14:53:59 UTC
(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.

Comment 32 hardt 2023-01-10 15:11:50 UTC
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.

Comment 33 Steve Traylen 2023-03-23 19:13:08 UTC
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  ?

Comment 35 Steve Traylen 2023-03-23 19:27:40 UTC
[fedora-review-service-build]

Comment 36 Jakub Kadlčík 2023-03-23 19:31:15 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/5701014
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-1997994-oidc-agent/fedora-rawhide-x86_64/05701014-oidc-agent/builder-live.log.gz

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.

Comment 37 Petr Vokac 2023-03-30 04:09:36 UTC
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 ***


Note You need to log in before you can comment on or make changes to this bug.