Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vendor: update to the latest systemd_ctypes #19927

Conversation

allisonkarlitskaya
Copy link
Member

@allisonkarlitskaya allisonkarlitskaya commented Feb 2, 2024

We didn't sync this in a while and there have been a lot of changes.

We also have a critical bug fix included: we recently introduced a test case (in 0bb7438) which triggers a segfault in the binding layer of systemd_ctypes due to FFI call trampolines being garbage collected while they're still running.

This is currently blocking downstream packaging on several non-x86_64 architectures.

@allisonkarlitskaya allisonkarlitskaya added the blocked Don't land until something else happens first (see task list) label Feb 2, 2024
@martinpitt
Copy link
Member

This won't be the final version of this PR, but FTR: I support this (wrt. to RHEL also). We've sold our souls on our tests telling us where we screwed up 😁

But I'd like to ask that we do a point release after landing this, and get them into RHEL ASAP, so that we get some field testing.

@martinpitt martinpitt removed the blocked Don't land until something else happens first (see task list) label Feb 2, 2024
@allisonkarlitskaya allisonkarlitskaya marked this pull request as ready for review February 2, 2024 08:43
@martinpitt
Copy link
Member

The previous draft version had two failures: aarch64 TF, which may be "randomly corrupted testbed" or some actual regression; and a weird i386 rpm build failure which doesn't look at all related to ctypes. But let's get this fully green.

martinpitt
martinpitt previously approved these changes Feb 2, 2024
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wholeheartedly "yes I want" if this gets green. Thanks!

New ruff warns to stderr that the old way is deprecated.  That's enough
to convince test/static-code that an error is being reported.

Let's move this over.
jelly
jelly previously approved these changes Feb 2, 2024
`--unix` has never been a valid command-line argument to `curl` but it
was accepted for the past decade because curl contained a "partial
match" feature to expand unambiguous prefixes to their full forms.

This feature was recently removed[^1] and a recent upload[^2] to rawhide
is causing breaks on packit.

Make sure we spell out the option name in full in all of the places we
use it.

[^1]: curl/curl@07dd60c
[^2]: https://bodhi.fedoraproject.org/updates/FEDORA-2024-b72488cc7d
Make sure we set the ->method field on ourselves fairly soon.  We might
need to change our "delayed response" behaviour depending on its value.
If we call cockpit_web_response_set_method() more than once, then we
leak the old value.  Fix it.
When creating the CockpitWebResponse make sure we pass the method
through.  This was being left as NULL in all sitautions except for
testcases.
This lets us clean up control flow a little bit — we don't need to let
the `l` variable outlive the scope of the loop, and in the error case we
can simply exit.

Also: move the computation of the error message closer to where we
actually send it.
The code in cockpit_web_response_error() sends a body, regardless of if
the request was for GET or for HEAD.  Let's fix it to only send the body
on GET.
We didn't sync this in a while and there have been a lot of changes.

We also have a critical bug fix included: we recently introduced a test
case (in 0bb7438) which triggers a
segfault in the binding layer of systemd_ctypes due to FFI call
trampolines being garbage collected while they're still running.

This is currently blocking downstream packaging on several non-x86_64
architectures.
@allisonkarlitskaya
Copy link
Member Author

A note about the webserver changes in this PR, for posterity:

They happened because the new curl version in rawhide is strict about servers not sending any payload when HEAD was requested. The RFC says that servers MUST NOT do this, so I guess they're right?

The commit that changed curl's behaviour was this one: curl/curl@d7b6ce6 (bisected locally against our test-socket-activation-helper.sh testcase) and it got included in the 8.6.0 release here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-634a6662aa

@allisonkarlitskaya allisonkarlitskaya merged commit b486774 into cockpit-project:main Feb 2, 2024
93 checks passed
@allisonkarlitskaya allisonkarlitskaya deleted the update-systemd_ctypes branch February 2, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants