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

Add a Freedesktop SDK based image #173

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tytan652
Copy link
Contributor

@tytan652 tytan652 commented Jan 2, 2024

Note

The new yarn.lock and dist/index.js inflate the change number of approximately 7 500 lines.

This PR implements some of the idea from #170.

  1. Replace Fedora base image by creating a Freedesktop SDK based image with Buildstream
    • The image is pushed to GHCR, my only questioning is about if all the images (one per runtime and arch) can be stored on it ?
    • Images with runtime pre-installed should be another PR after if this one get merged, same for documentation about using those images.
    • The buildstream project relies on mainly Freedestkop SDK and then gnome-build-meta as junction, mostly to avoid reinventing the wheel (e.g. xvfb).
    • SDK is used and not the Platform to give plenty of tooling since there is no package manager.
    • flatpak-builder is hold to version 1.3.3 since later version breaks the action on the screenshot part, switching to 1.4 can be done once the action is ready.
    • The GitHub CLI tool is installed since those images are mainly meant to GitHub CIs and the CLI is already installed in all runner images.
  2. Pre-includes the QEMU aarch64 static binary, to enable it the new update-binfmts action needs to used. This replace the need to install docker in a container and ran the setup-qemu-action.
    • I don't know if it was a good idea to do it directly in the flatpak-builder action, so it's a separated action.
    • Pre-built binaries are in use because building QEMU statically requires static elements and we have a storage limit on jobs.
  3. org.flatpak.Builder is replaced by the installation flatpak-builder-lint in the image.

The image is built, pushed and tested on my freedesktop_based_image_test branch

@bilelmoussaoui
Copy link
Member

What scares me on using a buildstream freedesktop based image, is the lack of any information to keep it up to date when needed.
It would be nice to add some small documentation on how it is supposed to be built locally/tested.

I will have a proper look at the PR tomorrow, thank you

@tytan652
Copy link
Contributor Author

What scares me on using a buildstream freedesktop based image, is the lack of any information to keep it up to date when needed.
It would be nice to add some small documentation on how it is supposed to be built locally/tested.

I had to learn some BuildStream basics but I'll try to make a little README in the buildstream folder.

@tytan652 tytan652 force-pushed the freedesktop_based_image branch 3 times, most recently from b0cd610 to 1679bec Compare January 22, 2024 11:55
Copy link

@alatiera alatiera left a comment

Choose a reason for hiding this comment

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

Other than Add update-binfmts action commit which I am not familiar with how the actions work much, everything else looks super good and nice. I dig the approach.

@@ -19,6 +27,9 @@ bst source track gnome-build-meta.bst

Both junctions are now update to the latest commit of their release branch

### Note for future update
- Freedesktop SDK can't be updated until flatpak-builder-lint supports lxml 5

Choose a reason for hiding this comment

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

We could bundle older lxml though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to limit how much the Github Action builds since we have not much space, so no deep override on the Freedesktop junction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Freedesktop SDK 23.08 broke the Python "API" by bumping lxml 4 to lxml 5, from far it does not look like a good thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you meant side-by-side instalation, I don't how to do that with pypi sources. It really looks like it is not possible.

buildstream/elements/tooling.bst Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Would be really cool to have flatpak-builder-lint in gnomeos/gnome-build-meta too! wink wink :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad idea, the tool is too unstable to have it on gnome.

Copy link
Contributor Author

@tytan652 tytan652 Feb 11, 2024

Choose a reason for hiding this comment

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

I'm even thinking about just installing poetry and some linter deps (e.g. desktop-file-utils), because of how much the linter changes (also lxml).

Edit: pip install poetry is fine to pick a specifix version of the linter, and the included linter is updated with its new runtime dependency.

Choose a reason for hiding this comment

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

That's fine, it will only be exposed in gnomeos which is building everything from main anyway.

But for CI then we should have both a pinned version of the tool to avoid breaking everyone using the action, as well as installing it at runtime so we only need to bump the version in the action, not rebuild the age too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lxml 4 is still a blocker to add it to GNOME OS since its deps use lxml 5 forevery version relying on newer 23.08 or later.

Choose a reason for hiding this comment

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

ditto as flatpak-builder-lint probably, fits nicely with the devel tooling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not versed on how to build this project from source, so I used a pre-built binary.

Choose a reason for hiding this comment

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

In gnome, we usually build non-gnome components from tarballs or git tags anyway. We only track main for gnome components we have ownership of

Choose a reason for hiding this comment

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

I want to steal this too now 😆

@alatiera
Copy link

I also wanted to replace the docker images we use in the GNOME CI flatpak template and I think we could look at moving all of this into a standardised gnomeos or fdsdk docker image along with the tools as they are super useful anyway. I'd suggest merging this and then we start moving things around to gnome-build-meta and see how it works out.

@tytan652 Would you be interested in this?

What scares me on using a buildstream freedesktop based image, is the lack of any information to keep it up to date when needed.

Feel free to ping me or any of the GNOME Release team or fdosdk maintainers. This would also be easier if we end up using the same image as I mentioned above, and will move maintenance to gnome-rt which is fine since it's infra.

@tytan652
Copy link
Contributor Author

I also wanted to replace the docker images we use in the GNOME CI flatpak template and I think we could look at moving all of this into a standardised gnomeos or fdsdk docker image along with the tools as they are super useful anyway. I'd suggest merging this and then we start moving things around to gnome-build-meta and see how it works out.

@tytan652 Would you be interested in this?

I'm not against trying to upstream elements to them, but:

  • Freedesktop SDK also did the same bad move as Fedora for flatpak-builder (providing unstable versions) which is one of the reason why this image was made.
  • flatpak-build-lint has proven to be really unstable, so I don't see it in such images.

@alatiera
Copy link

I'm not against trying to upstream elements to them, but:

  • Freedesktop SDK also did the same bad move as Fedora for flatpak-builder (providing unstable versions) which is one of the reason why this image was made.

I wasn't aware of this, we can probably fix it or tag a "stable" f-b release. Please file an issue!

@tytan652
Copy link
Contributor Author

tytan652 commented Feb 12, 2024

I wasn't aware of this, we can probably fix it or tag a "stable" f-b release. Please file an issue!

The main reason was dropping FUSE2, Freedesktop SDK should have move to 1.4 now this is out.
But the action is not ready for 1.4 behavior, only 1.2 (so 1.3.3).

@tytan652 tytan652 marked this pull request as draft February 12, 2024 10:04
@alatiera
Copy link

But the action is not ready for 1.4 behavior, only 1.2 (so 1.3.3).

Has anyone tried to port it to 1.4 and fuse3? Were there any issues?

@tytan652
Copy link
Contributor Author

Has anyone tried to port it to 1.4 and fuse3? Were there any issues?

#169 🤷

We will still need to pin the flatpak-builder version just in case a behavior change happen again.

@tytan652 tytan652 force-pushed the freedesktop_based_image branch 5 times, most recently from 1b95f80 to 13f5c00 Compare February 12, 2024 12:09
Hold flatpak-builder to version 1.3.3 to avoid breakage introduced in
later versions
@tytan652 tytan652 force-pushed the freedesktop_based_image branch 6 times, most recently from f0ccd47 to c4bd4c0 Compare February 12, 2024 15:38
Uses already compiled static build of QEMU to avoid reaching the storage
limit of the job

To simplify binfmt registering, update-binfmt (Debian tooling) is
installed and the needed template generated
Meant to be used in conjunction with the Freedesktop SDK based images to
enable QEMU architecture emulation for flatpak-builder using
update-binfmts and its templates.
@tytan652 tytan652 marked this pull request as ready for review February 12, 2024 16:13
@tytan652
Copy link
Contributor Author

tytan652 commented Feb 12, 2024

Refactored BuildStream workflows to be less prone to out of space issues.

Copy link

@alatiera alatiera Feb 13, 2024

Choose a reason for hiding this comment

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

You could avoid needing binfmt-support by adding a config in /etc/binfmt.d systemd should already be providing the binfmt service and it probably will work in the container too.

https://www.man7.org/linux/man-pages/man5/binfmt.d.5.html

Copy link
Contributor Author

@tytan652 tytan652 Feb 13, 2024

Choose a reason for hiding this comment

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

Systemd services is a no-go/op in Docker

Copy link
Contributor Author

@tytan652 tytan652 Feb 13, 2024

Choose a reason for hiding this comment

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

And binfmt requires privileged access

Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work, we use patches to appstream that are only available in org.flatpak.Builder, without those patches it's not usable. There is a cyclic dependency between the linter and org.flatpak.Builder.

Copy link
Contributor Author

@tytan652 tytan652 Apr 10, 2024

Choose a reason for hiding this comment

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

A problem that should be worked on and avoid to make it worse.

This PR was made before I discover, the mess that the appstream lib transition created.
So a BuildStream based solution will require upstreaming fine fixes.
It is out of question to duplicate patches and use them in this image rather than upstreaming.

Copy link
Contributor Author

@tytan652 tytan652 Apr 10, 2024

Choose a reason for hiding this comment

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

Also, I might drop the linter in the base image (and let the deps) since upstream only support running the master branch which is not good.
This base image is meant to be re-built only when we update/add something not all the time because of one tool.

Copy link
Contributor

@bbhtt bbhtt Apr 10, 2024

Choose a reason for hiding this comment

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

Yes, I'm suggesting to drop this as it doesn't work. Upstream doesn't agree with some of the patches and at the moment it is not possible to come to a solution.

Also the expectation is that Flathub raise or lower new appstream checks as per our needs - that's not possible to do without patches sometimes. ximion/appstream#604 (comment)

Next time, it's probably best to introduce a new check here at a low severity and have Flathub raise it (and only make AppStream raise it too after some time has passed).

Also the linter doesn't do tags now, it's deployed from commits of the master branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the expectation is that Flathub raise or lower new appstream checks as per our needs - that's not possible to do without patches sometimes.

Because you do not create your own libappstream validator with its API rather than patching their executables, I can understand that you don't have the bandwidth to do it for now…
But libappstream (including libappstream-compose) API were made to create appstream tooling more easily with less "reinventing the wheel" situation.

Copy link
Contributor

@bbhtt bbhtt Apr 10, 2024

Choose a reason for hiding this comment

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

The API too, doesn't allow lowering severity for most tags that we want to lower, afaik and it's out of the question right now because of maintainability reasons.

Choose a reason for hiding this comment

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

It's fine, we can build/bundle whatever version we need for flathub

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine, we can build/bundle whatever version we need for flathub

It specifically requires the patches that live in org.flatpak.Builder package, not just a version of appstream.

Choose a reason for hiding this comment

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

Yea but we can match whatever the setup for that is, it's purpose built images basically for that

Copy link
Contributor

Choose a reason for hiding this comment

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

If the patches are included then it is fine from me, otherwise without them the linter won't work at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants