-
Notifications
You must be signed in to change notification settings - Fork 113
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
Refresh Flatpak build workflow #9798
Conversation
544aa3b
to
0e6fa20
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.
I don't see anything concerning. I don't fully understand the logic in scripts/linux/script.sh
, but I assume you have tested it out.
Just a few nitpicks, but I don't see anything that should block merging.
|
||
build: | ||
name: "Build" | ||
runs-on: ubuntu-latest |
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'm a bit worried about using ubuntu-latest
. In releng stuff, we usually pin it to a specific major version.
Not a blocker, unless if you think this could cause problems in the future.
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 don't anticipate this would cause problems. Both the build
and linter
jobs are downloading and running containerized environments to do the real work, so we're not really making any significant use of the host Ubuntu environment.
linux/flatpak/appstream-metainfo.sh
Outdated
<name>Mozilla Corporation</name> | ||
</developer> | ||
|
||
$(python $(dirname $0)/appstream-releases.py | sed s'/^/ /g') |
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.
nitpick: Maybe move this outside the file content into a variable so it's more readable/easier to understand?
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.
The output of appstream-releases.py
is a fairly messy, multiline XML file which I'm not a fan of passing around as environment variables. And this script is really just a temporary workaround until something like this PR lands - after which we can replace all of this with a static XML file that links to a hosted releases file.
The gist of the problem that led to this hack, is that we are required to provide a <releases>
XML tag to pass Flatpak's submission guidelines - but we don't really have a good source of that release information. So we are trying to generate it dynamically from the git tags.
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.
The PR to mozillavpn-product-details has landed, so I have updated the PR to turn this into a static XML file that just links to the hosted releases file.
# Recompress the tarball | ||
case $1 in | ||
*.tar.gz) | ||
gzip -c $TMPDIR/archive.tar > $1 |
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.
should we use -9
to improve compression?
Edit: Also, how come we take in a file with extension .tar.gz
, but output to .tar
only?
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.
There is a little subtlety going on here, but we are making use of the -r
or --append
option to tar
here in an attempt to minimize the possible changes to the pip package's tarball. This simply appends a record to the archive to insert a single file without touching anything else.
The downside is that this option only works on uncompressed archives, which means we have to decompress the archive first (into a plain .tar
file) even though we don't need to extract the archive. Which is what these case statements are doing.
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.
The modification to this tarball is only done locally to the flatpak build directory, and the modified archive is never uploaded or stored anywhere. Therefore there isn't really any benefit to a more aggressive compression level.
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.
Awesome, thanks for explaining 😃
f57b1c4
to
261cd60
Compare
96ae2ba
to
528e349
Compare
d0a2632
to
971f3bc
Compare
Description
I've been thinking of giving the flatpak build infrastructure an update to try and pave the way for proper releasing on flathub. This PR attempts to lay the groundwork to build Flatpaks using the same environment that flathub uses, and develop a workflow that can update the pip/cargo dependencies and file pull requests to flathub.
Eventually the idea is that we can have a release job that automatically pushes tagged releases to flathub, and maybe even release candidates to the flathub beta.
For a quick tour of what's changed:
scripts/linux/script.sh
), this requires that:flatpak-cargo-generator.py
script to generate a manifest for the Rust crates used by the VPN project.glean_parser
using Flatpak manifests. This was a pain because therpds
dependency is a mixed Python/Rust project and it requires some hacks to get it building offline.flatpak-pydeps.yaml
as it doesn't add anything of value to the build.Some future work:
Flathub requires us to include a<releases>
element in the Appstream data, but we really don't have a single source of truth for our releases. At present, this is dumping the tags off github and gluing it together with the Github releases API. A better approach would be to host the releases file using something like this and then replace it with a URL reference.releases/x.y.z
branches to Flathub beta would be better.Reference
JIRA issue: VPN-6302
Checklist