Bug 2037863 - Review Request: libsoup3 - Soup, an HTTP library implementation
Summary: Review Request: libsoup3 - Soup, an HTTP library implementation
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Michael Catanzaro
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 2037550
TreeView+ depends on / blocked
 
Reported: 2022-01-06 17:20 UTC by Gwyn Ciesla
Modified: 2022-01-26 17:51 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2022-01-19 20:21:40 UTC
Type: Bug
Embargoed:
mcatanza: fedora-review+


Attachments (Terms of Use)

Description Gwyn Ciesla 2022-01-06 17:20:05 UTC
Description:
Libsoup is an HTTP library implementation in C. It was originally part
of a SOAP (Simple Object Access Protocol) implementation called Soup, but
the SOAP and non-SOAP parts have now been split into separate packages.

libsoup uses the Glib main loop and is designed to work well with GTK
applications. This enables GNOME applications to access HTTP servers
on the network in a completely asynchronous fashion, very similar to
the Gtk+ programming model (a synchronous operation mode is also
supported for those who want it).

SPEC: https://fedorapeople.org/~limb/review/libsoup3/libsoup3.spec
SRPM: https://fedorapeople.org/~limb/review/libsoup3/libsoup3-3.0.3-1.fc35.src.rpm

Comment 1 Fabio Valentini 2022-01-06 18:59:57 UTC
Isn't this backwards?

The "main" package should be updated to the newest version, and compat packages CAN be created for older versions.
See the Naming Guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#multiple

i.e. update the main "libsoup" package to version 3, and then create a libsoup2 compat package for the older 2.x branch.
(if you do it that way, you're also eligible for automatic review exception, so you won't need a package review at all:
https://docs.fedoraproject.org/en-US/packaging-guidelines/ReviewGuidelines/#_package_review_process , bullet point 2)

Comment 2 Gwyn Ciesla 2022-01-06 19:07:13 UTC
That's what I thought, but https://bugzilla.redhat.com/show_bug.cgi?id=2037557#c1

Comment 3 Fabio Valentini 2022-01-06 19:09:35 UTC
In my opinion, Milan is wrong to make you create a new package.
But I also think the packaging guidelines could actually be more clear in this regard ...

Comment 4 Fabio Valentini 2022-01-06 19:18:48 UTC
https://pagure.io/packaging-committee/issue/1150

Comment 5 Milan Crha 2022-01-07 07:32:59 UTC
(In reply to Fabio Valentini from comment #3)
> In my opinion, Milan is wrong to make you create a new package.

Well, the two versions are parallel installable, but cannot be used from a single binary. I see a similarity with gtk2/gtk3/gtk4, or enchant/enchant2. You still want to allow build against libsoup2 - I understood the compat packages are only for runtime, at least mostly/usually.

I can be wrong, of course.

There is a slow ongoing effort to port applications (and their dependencies) to libsoup3 [1], but no hurry on that side. It's tricky, because the dependencies can be indirect (your dependency can use libsoup3, but a library it uses, which uses another library, is not ported or built to libsoup3). Once *all* the applications and libraries are ported the libsoup2 will eventually be removed from the Fedora, but that too far in the future at the moment.

Michael (CC'ed) might have some more insight on the effort of getting libsoup3 into Fedora.

[1] https://gitlab.gnome.org/GNOME/libsoup/-/issues/218

Comment 6 Michael Catanzaro 2022-01-07 15:32:49 UTC
Well of course they need to be two different packages. The question here is which option do we pick:

 * libsoup package contains libsoup 2, new libsoup3 package contains libsoup 3
 * Or: libsoup package contains libsoup 3, new libsoup2 compat package contains libsoup2

If the current package was named "libsoup2" then I would say the path of least resistance would be a new libsoup3 package. But it's really just called "libsoup," so it makes sense to me to go with Fabio's suggestion: upgrade that to libsoup3 and provide libsoup2 as a compat instead. Of course this will break some dependent packages that explicitly BuildRequires the libsoup package and expect libsoup2, but anything that uses pkgconfig for its BuildRequires will be perfectly fine.

On the other hand, using a new package for libsoup3 would certainly be *easier* since it would avoid mass breakage (although the transition is still going to be tough). I think either option is perfectly acceptable.

> Michael (CC'ed) might have some more insight on the effort of getting libsoup3 into Fedora.

It is finally safe to try this because libsoup3 has been updated to crash if the process is linked to libsoup2, and libsoup2 has been updated to crash if the process is linked to libsoup3. But honestly, it's going to require a major coordination effort. For now, I recommend disabling libsoup3 support in package builds, and not upgrading to new software versions that require it.

> You still want to allow build against libsoup2 - I understood the compat packages are only for runtime, at least mostly/usually.

Hm, I think it's up to us whether we provide libsoup2-devel or not. Eventually we will want to cut off packages to prevent them from building against libsoup2, but surely it's too soon for that. We still support glib (glib1) and gtk (gtk1), after all, which seem like more important targets to remove.

Comment 7 Michael Catanzaro 2022-01-17 17:14:13 UTC
After thinking about this more: using a new package is consistent with how most other GNOME packages handle major API breaks (e.g. GLib, GTK, libgweather) when the new package is intended to be parallel-installable and live alongside the old package for a long time. This is in contrast to something like OpenSSL where we want to upgrade the whole distro to the new version ASAP. SO I think this is OK.

Comment 8 Fabio Valentini 2022-01-17 17:21:48 UTC
> So I think this is OK.

Yes. I apologize for the confusion. We discussed this during last week's FPC meeting:
It is not explicitly stated in our guidelines for this case, but the non-versioned package does not necessarily have to be the one for the newest version.

Comment 9 Michael Catanzaro 2022-01-17 17:57:50 UTC
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- If your application is a C or C++ application you must list a
  BuildRequires against gcc, gcc-c++ or clang.
  Note: No gcc, gcc-c++ or clang found in BuildRequires
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
- Latest upstream version is now 3.0.4, would be good to update to that
- Please use rpmautospec %autorelease and %autochangelog from the beginning, so we don't have to convert the package later on.
- Instead of BuildRequires: glib2-devel >= %{glib2_version}, you should depend on pkgconfig(glib-2.0) and pkgconfig(gio-2.0).
- Is the xgettext.patch upstream? Can you add a link to the upstream commit or MR just above it in the spec file?
- I think it should Recommends: glib-networking, not Requires. Requires is not quite true because it is perfectly possible to use libsoup without TLS support. That's not what most users want to do, but it's what Recommends is for.
- Similarly, there's no need to Requires: glib2 because that is redundant with the automatic shared object dependency.
- The package description should end with ", but the SOAP parts were removed long ago."
- The # %%{with_docs} comment is trying to be helpful, but there is so little space since the start of the condition, that it really only clutters the spec. I would remove that.
- Need to use %global instead of %define throughout

Most of these are preexisting issues with the original libsoup.spec. I suppose it's good to re-review spec files every so often.

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[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.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[!]: Package does not own files or directories owned by other packages.
     Note: This is a fail, but I've never understood this requirement. It seems to conflict with the packaging guidelines, which require the package to own any directory it creates that might not be owned by a dependency. So I would just ignore this.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: The spec file handles locales properly.
[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.
[!]: Requires correct, justified where necessary.
     Note: see issues at top of review
[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.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 163840 bytes in 3 files.
[x]: Package complies to the Packaging Guidelines
[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 requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[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]: Macros in Summary, %description expandable at SRPM build time.
[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]: 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:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[!]: Final provides and requires are sane (see attachments).
     Note: see issues at top of review
[x]: Package functions as described.
[!]: Latest version is packaged.
     Note: latest version is now 3.0.4
[x]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
     Note: see issues at top of review
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
     Note: this is a good fail. Please don't add %check. We want to test packages in gating, not during %check.
[?]: Packages should try to preserve timestamps of original installed
     files.
[!]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define glib2_version 2.58.0,
     %define gtkdoc_flags -Dgtk_doc=true, %define gtkdoc_flags
     -Dgtk_doc=false
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[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]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.

===== EXTRA items =====

Generic:
[-]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
     Note: Arch-ed rpms have a total of 1515520 bytes in /usr/share
[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]: 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://download.gnome.org/sources/libsoup/3.0/libsoup-3.0.3.tar.xz :
  CHECKSUM(SHA256) this package     : 5165b04dadae3027e9a2882d868694b4586affd778c194982ae4de2373d2e25e
  CHECKSUM(SHA256) upstream package : 5165b04dadae3027e9a2882d868694b4586affd778c194982ae4de2373d2e25e


Requires
--------
libsoup3 (rpmlib, GLIBC filtered):
    glib-networking(x86-64)
    glib2(x86-64)
    libbrotlidec.so.1()(64bit)
    libc.so.6()(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)
    libgssapi_krb5.so.2()(64bit)
    libgssapi_krb5.so.2(gssapi_krb5_2_MIT)(64bit)
    libnghttp2.so.14()(64bit)
    libpsl.so.5()(64bit)
    libsqlite3.so.0()(64bit)
    libz.so.1()(64bit)
    libz.so.1(ZLIB_1.2.0)(64bit)
    rtld(GNU_HASH)

libsoup3-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    libsoup-3.0.so.0()(64bit)
    libsoup3(x86-64)
    pkgconfig(gio-2.0)
    pkgconfig(glib-2.0)
    pkgconfig(gmodule-2.0)
    pkgconfig(gobject-2.0)
    pkgconfig(libbrotlidec)
    pkgconfig(libnghttp2)
    pkgconfig(libpsl)
    pkgconfig(sqlite3)
    pkgconfig(sysprof-capture-4)
    pkgconfig(zlib)

libsoup3-doc (rpmlib, GLIBC filtered):

libsoup3-debuginfo (rpmlib, GLIBC filtered):

libsoup3-debugsource (rpmlib, GLIBC filtered):



Provides
--------
libsoup3:
    libsoup-3.0.so.0()(64bit)
    libsoup3
    libsoup3(x86-64)

libsoup3-devel:
    libsoup3-devel
    libsoup3-devel(x86-64)
    pkgconfig(libsoup-3.0)

libsoup3-doc:
    libsoup3-doc

libsoup3-debuginfo:
    debuginfo(build-id)
    libsoup-3.0.so.0.0.3-3.0.3-1.fc36.x86_64.debug()(64bit)
    libsoup3-debuginfo
    libsoup3-debuginfo(x86-64)

libsoup3-debugsource:
    libsoup3-debugsource
    libsoup3-debugsource(x86-64)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 2037863
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, C/C++, Generic
Disabled plugins: SugarActivity, Python, PHP, fonts, Java, Ocaml, Perl, Haskell, R
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comment 10 Gwyn Ciesla 2022-01-18 22:44:19 UTC
I'm working on all of the above. The xgettext patch is to work around a bug I've not yet totally run to ground. I'll send upstream if appropriate.

Additionally, this is my first %autorelease/%autochangelog attempt. This seems like something that will work well inside a fedpkg repo, but I'm working on this in a simple rpmbuild tree. What should I call the file in ~/rpmbuild/SOURCES/ so that it's included?

Comment 11 Fabio Valentini 2022-01-18 22:49:30 UTC
I would recommend only switching to use rpmautospec macros once you import the package into a dist-git repository.
The macros make all kinds of assumptions that are just not broken or wrong outside a dist-git repo.

As you have noticed, they don't work with classic "~/rpmbuild/{SOURCES,SRPMS,SPECS}" directory tree layout at all, and unless you work inside a git repository, the Release and changelog will always evaluate to their fallback values:

rpm -E %autorelease
1.fc35

and

rpm -E %autochangelog
* Tue Jan 18 2022 John Doe <packager> - %{version}-%{release}
- local build

This is almost certainly not what you want.

> What should I call the file in ~/rpmbuild/SOURCES/ so that it's included?

Which file are you talking about?

Comment 12 Gwyn Ciesla 2022-01-18 22:53:35 UTC
The changelog file. In the docs I've found it just refers to a file named 'changelog' which will be subject to name collisions in rpmbuild.

Comment 13 Fabio Valentini 2022-01-18 22:56:50 UTC
(In reply to Gwyn Ciesla from comment #12)
> The changelog file. In the docs I've found it just refers to a file named
> 'changelog' which will be subject to name collisions in rpmbuild.

Yeah, you'll need that in the same directory as your .spec file.
Which is one of the reasons why the classic rpmbuild/SPECS/ layout won't work here.

I also wonder why you're using that file at all?
It's only used for overriding previous entries if they contained mistakes, or for keeping around old changelog entries of existing packages if they opt in to rpmautospec later.

Comment 14 Michael Catanzaro 2022-01-18 23:39:51 UTC
To be clear: the changelog file is not required for new packages. That's created one time when migrating a pre-rpmautospec package to use rpmautospec.

Comment 15 Gwyn Ciesla 2022-01-19 19:40:05 UTC
Ignorance?

SPEC: https://fedorapeople.org/~limb/review/libsoup3/libsoup3.spec
SRPM: https://fedorapeople.org/~limb/review/libsoup3/libsoup3-3.0.3-1.fc35.src.rpm

It would be lovely it it supported $SRPMNAME-changelog but oh well.

Comment 16 Gwyn Ciesla 2022-01-19 20:05:14 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libsoup3

Comment 17 Gwyn Ciesla 2022-01-19 20:21:40 UTC
Thank you!

Comment 18 Milan Crha 2022-01-26 11:54:37 UTC
I thought I'll click on something on the https://src.fedoraproject.org/rpms/libsoup3 (I'd bet that was the only way to get to the package co-maintaining in the (distant) past), but I cannot find anything like that there, thus I'm writing here:

If you'd like to help with the updates/upstream releases and such, then feel free to add me to the list of the Members. The login is 'mcrha'.

Comment 19 Gwyn Ciesla 2022-01-26 17:51:11 UTC
Added, thank you!


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