port to libsoup3

[q66]

Hello,

this is an initial port of libostree to use libsoup3. The reason for this is:

1. GNOME and webkit are migrating to libsoup3, and other upstreams should do so as well as the new generation is significantly cleaned up, brings http/2 support and other things
2. libsoup2 and libsoup3 cannot exist in the same process together
3. libsoup2 is in maintenance mode and everything is expected to migrate over time

This pull request does several things:

1. Upgrades glib requirement to at least 2.68 2.66 (though it does not yet actually fix up all the warnings that result from this - it only fixes up the bare minimum to get it to build)
2. changes all usage of SoupURI to GUri (which is needed for libsoup3) - this is why the above is needed
3. actually ports the fetcher as well as the server
4. removes the soupuri copypasta from the build (though does not yet remove the actual files)

For now this is rough, as I wanted to get comments first. Notably, I was wondering wthether to attempt some kind of dual-libsoup compatibility. I have already attempted this, but it's fairly complicated and it would take a lot of effort to coerce the code to work both ways (with a lot of stuff being wildly different between the generations). This supports both versions of libsoup now. Additionally, I wanted to find out how the maintainers feel about bumping up the dependency requirements (glib), as that is a significant thing to consider. If this is okay, it would probably be a good idea to start by updating the glib requirement in the codebase on its own first.

It is expected that this rework is buggy for now. I wanted to get this out so that people are aware of it, and so that somebody else doesn't start working on it and waste effort.

In any case, I'm looking forward to this getting a review.

[openshift-ci]

Hi @q66. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[cgwalters]

/ok-to-test

[cgwalters]

Upgrades glib requirement to at least 2.68

From the ostree-for-Fedora/RHEL ecosystem...I think we can deal with this. The glib in rhel8 is now quite old, but...we can take the hit of cherry picking patches there instead; that's our burden to carry.

An important point here is that today in Fedora derivatives, ostree uses libcurl, not libsoup. So if we can potentially avoid bumping the glib dependency if libcurl is used, that'd be helpful.

That aside, this looks good to me so far! Thanks for working on this!

[q66]

The reason the dependency is bumped is the migration to GUri; SoupURI is gone in libsoup3, and the copy-pasted SoupURI inside the codebase appears to be pretty much a workaround. GUri as it is requires at least 2.66, while 2.68 is necessary for G_URI_FLAGS_SCHEME_NORMALIZE.

So the options are basically:

1. just switch to GUri as it is right now; that means dropping a bunch of code even for libcurl, and is likely better going forward, as long as you can deal with the newer version
2. keep the copypasted SoupURI; enable the copypasted SoupURI even for libsoup backend, add internal code for conversion to GUri where libsoup needs it; change the build system to only need newer glib when compiling libsoup (because regardless of how things done, newer libsoup will pull in newer glib requirement because it uses GUri in its public API), and somehow deal with the glib version incompatibilities (which will likely be its own can of worms because it'd need ifdefs or wrappers around the codebase)

[cgwalters]

just switch to GUri as it is right now; that means dropping a bunch of code even for libcurl, and is likely better going forward, as long as you can deal with the newer version

Yeah, SGTM. But I don't want to speak for everyone - this definitely has implications for the flatpak ecosystem. There's a somewhat related thread over here: flatpak/flatpak#2241 (comment)

So summoning @smcv

[mwleeds]

I think the related Flatpak PR is flatpak/flatpak#4582 unless I'm missing something.

[smcv]

GLib 2.68 is going to be available in Ubuntu 22.04 (coming soon) and Debian 12, but a dependency on GLib 2.68 would rule out backporting libostree to Ubuntu 20.04 (the current LTS right now) and Debian 11.

If it's possible to limit the dependency to 2.66, then that would open up the option of backporting to Debian 11, which would be nice. An weak build-time dependency with GLIB_CHECK_VERSION is absolutely fine.

At the moment, all backport and PPA builds of Flatpak use libostree 2020.8, which is the version shipped in Debian 11, and that has been sufficient so far (although there are some annoying bugs for which I might try to backport patches at some point).

However, if Flatpak wants to start depending on new features of libostree 2022+, then a GUri dependency would make that awkward.

The other side of this is that Flatpak itself uses libsoup directly. If Flatpak and libostree are ported to libsoup 3, distros that compile libostree against libsoup will need to be able to switch both of them from libsoup 2 to libsoup 3 as a flag-day, because libsoup doesn't have versioned symbols and so mixing the two libraries will crash.

At the moment, the .deb world is still building libostree against libsoup, because it is not clear to me what the advantages and disadvantages of libsoup vs libcurl are, so I stuck with the status quo instead of making arbitrary changes; and the Ubuntu and PPA packages are derived from my Debian packages, so they inherit that policy. If there are reasons why I should switch to libcurl, that's entirely possible to do.

[q66]

I thought distros in general used libsoup for ostree, but as it turns out Fedora/RHEL does not, so it should be practical for things that do not switch flatpak to libsoup3 to just use the libcurl backend for ostree

I believe 2.66 should be doable, so if that helps, I can attempt that...

[dbnicholson]

At Endless we use libsoup in ostree and I don't think we'd switch over to libcurl without a lot of testing. Even though I don't have any reasons to doubt libcurl, download failures == you can't upgrade failures. The ostree/flatpak libsoup flag day does not sound fun, though.

[smcv]

Notably, I was wondering wthether to attempt some kind of dual-libsoup compatibility. I have already attempted this, but it's fairly complicated and it would take a lot of effort to coerce the code to work both ways (with a lot of stuff being wildly different between the generations)

If the same libostree codebase cannot work with both libsoup 2 and libsoup 3, then that effectively means distros that use libsoup in libostree won't be able to upgrade libostree to a libsoup3 version until Flatpak is ready for it.

[q66]

yes, indeed - i was thinking those could switch to curl backend for the time being, or alternatively carry a patch for flatpak (there is already an open pull request in flatpak for that, it was linked earlier - it appears quite ready, and is a trivial patch, much simpler than the ostree port itself)

of course, the ideal case would be having support in both projects merged, so nobody has to carry any patches...

[q66]

I added back support for libsoup2; it can be enabled with --with-soup2. The higher GLib requirement is still in place (I will try to get it down to 2.66 though) as GUri is still used in ostree-fetcher-uri so that the copypasta can be dropped and so that the work becomes easier for soup3 support. The soup2 support is written against relatively modern soup2 API (i.e. all the deprecated conditional stuff is gone, I set the baseline to 2.70.0 which matches Ubuntu 20.04) and to share as much as possible with the soup3 support, it does not use SoupRequest and the likes.

This should address things for distributions that will not be switching to soup3 yet but also don't want to switch to curl.

[q66]

changed minimum glib version to 2.66; updated the GUri code to only use the 2.68 scheme normalization when new enough and otherwise it normalizes manually

[dbnicholson]

Can you split out the GUri change? It seems useful on its own and a necessary prerequisite for the rest of the work.

After that, is there any reason soup3 can't be a separate fetch backend instead of replacing the soup2 one? The hard work of abstracting the fetch backend has already been done, so I don't see a good reason not to leave the soup2 one alone. That would allow ostree and flatpak to migrate in a much cleaner way and let consumers using the soup2 backend continue to do so. At some point in the future the soup2 backend could be dropped.

[q66]

well, at this point soup2 and soup3 share a lot of code; if you prefer that soup2 code remains mostly untouched and soup3 code is made separate, I can do that too

i was planning to split the GUri changes all along, so sure

[dbnicholson]

I personally think you should leave the soup2 code entirely alone. That ensures there are no regressions introduced and removing the backend later is simpler. At that point any duplication just goes away and there are no leftover abstraction layers from soup2/3 code sharing. Maybe other ostree developers feel differently, though.

[q66]

yeah, sounds good; let's do that

[cgwalters]

Dan's suggestion SGTM! It will definitely be easier for distributors to avoid a chained flag day that way.

(Although, have the recursive dependencies of libflatpak been analyzed? e.g. does gnome-software also use libsoup directly? I'd be surprised if it didn't.)

[q66]

I've previously compiled a more or less complete list of all software that uses libsoup (https://gitlab.gnome.org/GNOME/libsoup/-/issues/218); we're gradually helping upstreams port things over

i'm also working on a linux distribution that currently uses libsoup3 only, so that gets me a testbed to verify things

[q66]

ok, first part of the work is at #2551

[q66]

this is now rebased on the GUri work; the soup3 backend is added as a separate thing

i adjusted the CI to use soup2, unfortunately it's not available in Debian yet - I should figure out a way to get it tested (as there are probably bugs right now)

the httpd is ported to support both versions of libsoup at once

[dbnicholson]

as far as i can tell, the soup session is created in the thread in the new soup3 fetcher, and all api usage is already async? or do you see something that i have missed?

I see this in some of the test logs:

# libsoup:ERROR:../libsoup/soup-session.c:1458:message_completed: assertion failed: (item->context == soup_thread_default_context ())
# Bail out! libsoup:ERROR:../libsoup/soup-session.c:1458:message_completed: assertion failed: (item->context == soup_thread_default_context ())

I think if that can be fixed and the test suite passes, then I think this can go in as a first cut. However, I think it would be better to not use threads, right? Then you avoid any of the soup3 thread gotchas and you get the HTTP/2 benefits.

Can I push to your branch? I guess I could make a PR on top of it with my changes.

[dbnicholson]

Some nitpicks on the build side. I'd prefer duplication as separate options rather than having soup2 vs soup3 be treated the same just like how the fetch backends are separated. That will likely require some duplication of build settings, but I'd rather have that then having them intertwined. ostree-trivial-httpd makes this more awkward, but I'm going to treat that as a special case.

[q66]

alright, i will rebase later, if you can't push into my PR (i know gitlab allows it but github might not), you could just post a git patch and i can apply it myself

the log indeed seems to indicate wrong usage, I'll see what i can do (unfortunately i have much less time to work on this than before)

[dbnicholson]

alright, i will rebase later, if you can't push into my PR (i know gitlab allows it but github might not), you could just post a git patch and i can apply it myself

What do you know, I could push. Feel free to squash those 3 patches into yours or polish the commit messages if you prefer. I also ran into dd98a2a, which would go away on a rebase.

[dbnicholson]

I added a debian testing test using --with-soup3. We'll see how it goes.

[dbnicholson]

Well, it wasn't going to run the tests without a rebase, so I did it and force-pushed back to your branch. Locally I only have 2 errors left:

ERROR: tests/test-pull-large-metadata.sh
========================================
...
# **
# libsoup:ERROR:../libsoup/soup-session.c:1458:message_completed: assertion failed: (item->context == soup_thread_default_context ())
# Bail out! libsoup:ERROR:../libsoup/soup-session.c:1458:message_completed: assertion failed: (item->context == soup_thread_default_context ())

I'm not sure what's going on there.

ERROR: tests/test-remote-cookies.sh
===================================
...
+ ostree --repo=repo pull origin main
error: Server returned status 403: Forbidden
...
+ ostree --repo=repo pull origin main
error: Could not connect to 127.0.0.1: Connection refused

The connection refused happens after trivial-httpd dies. Unfortunately, trivial-httpd's stderr gets closed when it's daemonized, so it's hard to tell what's going on.

[dbnicholson]

I fixed all the soup2 regressions, but there are still piles of soup3 test errors to comb through. Part of it has to do with dconf that I'm guessing soup3 pulls in.

[dbnicholson]

With a couple environment variables stolen from libsoup's test suite, it's down to one failing test:

ERROR: tests/test-pull-large-metadata.sh
========================================
...
# libsoup:ERROR:../libsoup/soup-session.c:1458:message_completed: assertion failed: (item->context == soup_thread_default_context ())

[dbnicholson]

I squashed my fixups into @q66's patch. I then decided to give the full single thread async approach. I think it turned out nicely and (IMO) is simpler. And it got rid of the last test failure.

@cgwalters or @jlebon either of you care to review?

[cgwalters]

Looks sane to me, thanks!
I do still prefer declare-and-initialize for new code, but it's fine.

he fixed it

[dbnicholson]

Looks sane to me, thanks! I do still prefer declare-and-initialize for new code, but it's fine.

I was fixing those as I noticed them. Let me take one more sweep to get them all.

[dbnicholson]

I think I cleaned up all the non-declare-and-initialize and change most declarations to be at the site of use.

[dbnicholson]

A couple things I noticed while working on this that could use followups:

• While soup3 supports HTTP/2, the only way to disable it prior to 3.3.0 is the SOUP_FORCE_HTTP1 environment variable. In 3.3.0, soup_message_set_force_http1 was added. Currently OSTREE_FETCHER_FLAGS_DISABLE_HTTP2 is unsupported in the backend.
• The soup2 fetcher has some validation that compares the written content size to the Content-Length header that was copied over to the new fetcher. For a bit I accidentally had the compression settings reversed and was getting Download incomplete errors fetching from our server where gzip compression is supported. That makes sense as Content-Length is unreliable if the content has been transformed on retrieval. The validation was actually checking if the size was at least Content-Length bytes, so it usually wouldn't fail since the decompressed size is usually greater than the compressed size. Except if you're compressing very small files and then the decompressed size may actually be less than the compressed size. The fetcher should only perform the Content-Length validation if the content wasn't decoded. This is a bug in the soup2 fetcher that should be fixed. In the soup3 fetcher this was trickier since the SOUP_MESSAGE_CONTENT_DECODED flag was removed. In that issue it was pointed out that SoupMessageMetrics in soup3 would be a better match since we can compare the written size to the actually read size and assume that soup is already validating Content-Length.

[dbnicholson]

Thanks for getting this going @q66!

[q66]

thanks a lot for fixing this up, unfortunately i wasn't able to find the time to look at it myself