-
Notifications
You must be signed in to change notification settings - Fork 68
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
migrate to new platform-cache-workflow
to manage Dockerfile caching in CI
#1703
Conversation
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.
LGTM
@@ -79,14 +79,13 @@ jobs: | |||
|
|||
dev-package: | |||
name: Build leap-dev package | |||
needs: [platforms, build-base] | |||
if: always() && needs.platforms.result == 'success' && needs.build-base.result == 'success' | |||
needs: [platform-cache, build-base] | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
platform: [ubuntu20, ubuntu22] |
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.
Just a thought, while doing this update should we update the platform
s in the matrix:
sections throughout our workflows to be dynamic from the platform-cache-workflow
like done in ph_backward_compatibility
?
platform: ${{fromJSON(needs.platform-cache.outputs.platform-list)}}
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.
It looks like that is the right approach now, but that's only because there is a 1:1 between all the platform files and all the platforms we're going to build leap-dev on. That may not always be true. For example, we may choose not to support building on Ubuntu 20, but still support running the reproducible build on Ubuntu 20. In such a scenario we'd still need a ubuntu20
platform defined in the platforms
/platform-list
, but its purpose would only be for testing. So using platform-list
here wouldn't work.
(I expected to already need to make use of this for the reproducible builds: I expected I'd need a special ubuntu20+cmake3.27 platform to run ctest
on since cmake3.27 is what is used to build. Shockingly, that doesn't appear to be the case)
I feel like we probably need some sort of .json file that gets sucked in at the start of the workflow that defines these copy pasted repetitive items like [ubuntu20, ubuntu22]
. (It gets more unwieldy with the upcoming reproducible workflow additions)
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.
oh, hm, I see I did plumb that through elsewhere as platform-list
..
platform-list: '["${{github.event.inputs.platform-choice}}"]' |
leap/.github/workflows/build.yaml
Line 50 in 381b3ed
platform-list: ${{needs.platform-cache.outputs.platform-list}} |
In this case here, I guess it could be changed to
platform-list
for consistency
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.
Actually those are from the work I did earlier to use platform-matrix
which you renamed to platform-list
in this PR with your move to the platform-cache-workflow
.
I'm fine with leaving the [ubuntu20, ubuntu22]
was just asking since this PR was working in that space. You called out a good example where this won't be 1:1, so that's good enough of a reason to leave it. I just wish there was a way to not have it hardcoded everywhere. It makes updating the workflows a bit tedious and error prone when adopting or dropping a platform.
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.
We should come up with something better; but at least let's look at how the reproducible one works before noodling on it more. (and, at the danger of overthinking future proofing, how something like an ARM build or a macOS build would look)
Had trouble running the |
filled out PR description some more. proper docs in https://github.com/AntelopeIO/platform-cache-workflow will need to wait another day or two |
Migrate from existing in-repo platforms.yaml+discover-platforms-action to a new platform-cache-workflow. Work on reproducible builds was primary motivator for change; I want to separate out the concept of caching platform build environments away from what the combinations of platforms leap builds with (since platforms may no longer be 1:1 with what gets built and tested).
Besides this additional encapsulation, new improvements/benefits are,
if: always()
nonsense and makes the workflows cancelable again(!), but comes at the price of some potential issues with ENF runners that will just have to be evaluated in practice.platforms.json
and expects to be passed platform files as a workflow parameter; directories are allowed where it will discover all files in the directory and use each file found as a platform (as being done here in this PR). The base filename of the file will be used as the platform name.will become
So even though
reproducible.Dockerfile
defines multiple stages, only thebuild
stage is used and cached.work on #1641