-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
tools: Enable Python bridge on Fedora 38 #19009
Conversation
dbb55d0
to
6a162c2
Compare
6a162c2
to
4674125
Compare
Oh, this is interesting -- it also enables the pybridge on Fedora CoreOS, which apparently needs some work. |
4674125
to
a02c31c
Compare
a02c31c
to
6caa72c
Compare
Could you please do an initial review of this? I think I want to clean up the "memory eater" commit more, but I'd appreciate @allisonkarlitskaya 's gut feeling/judgement about that. Also, @mvollmer, I suppose there was some particular reason for the Thanks! |
6caa72c
to
6cea6d5
Compare
1769f28
to
e130840
Compare
src/cockpit/packages.py
Outdated
@@ -317,11 +319,13 @@ def get_content_security_policy(self, origin): | |||
def serve_file(self, path, channel): | |||
if path == 'po.js': | |||
# We do locale-dependent lookup only for /po.js. This always succeeds. | |||
# Keep the file in memory for efficient lookups and manifests.js processing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this makes the types here unnecessarily complicated now. It would probably be fine to go all the way and drop the translations from memory as well.
e130840
to
895a545
Compare
895a545
to
d24b895
Compare
This comment was marked as resolved.
This comment was marked as resolved.
48401d9
to
57cc038
Compare
b26c142
to
4195a08
Compare
4195a08
to
3a36307
Compare
Fixed the conflict and dropped the memory saver patch for now. This will most probably fail the /devel scenario again, but let's make sure everything else still works. |
The test actually looks a lot better now, as commit 1e191f9 already removed half of the RAM usage. So my proposal is to work around that by giving the provisioned machine a bit more memory than 512 MiB in the devel scenario. This shouldn't be a big practical problem as real users are going to use the small production version of the pages. That would allow us to get some more field testing. @allisonkarlitskaya what is your gut feeling, should we still block this on your packages.py rework, or are you okay with unleashing it with the next release in two weeks? |
Give me another day on the packaging rework (and maybe start initially reviewing). It's not far now. |
dbeda3c
to
dacb340
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Fascinating!! The python bridge changes pixels in a way that we otherwise didn't notice from the integration tests yet. This definitely needs looking into. |
We've been running pixel tests on /pybridge for a while now, to spot exactly such causes; but it happens with the C bridge as well. This is a known flake, retrying. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a known flake, retrying.
aww too bad :)
+1, then.
cockpit-project/cockpit#19009 switched Fedora 38 to the pybridge for the "regular" build. Still keep the pybridge scenario for the other projects, until cockpit with the above PR got released and the fedora-38 image refreshed with it.
cockpit-project/cockpit#19009 switched Fedora 38 to the pybridge for the "regular" build. Still keep the pybridge scenario for the other projects, until cockpit with the above PR got released and the fedora-38 image refreshed with it.
After this, we can remove the fedora-38/pybridge scenario.
Move to Python bridge in Fedora 38 and CentOS Stream 9
The rewritten Python bridge that was introduced in Cockpit 294 gets enabled on Fedora 38 and CentOS Stream 9. Please let us know if you run into any trouble!