-
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
[5.0] integrate reproducible build with CI Build & Test workflow #1710
Conversation
If running the See: https://github.com/AntelopeIO/leap/actions/runs/6398790634/job/17369675575
Similar failure seems to happen with the
|
For PH workflow, it seems there will be a problem where over the course of time newer platforms will become available that aren't previously supported. If it wasn't for the - if: needs.v.outputs.leap-target != 'DEFAULT' && (startsWith(needs.v.outputs.leap-target, "3") || startsWith(needs.v.outputs.leap-target, "4"))
name: Download Prev Leap Version
uses: AntelopeIO/asset-artifact-download-action@v3
with:
owner: AntelopeIO
repo: leap
target: '${{needs.v.outputs.leap-target}}'
prereleases: ${{fromJSON(needs.v.outputs.leap-prerelease)}}
file: 'leap.*${{github.event.inputs.platform-choice}}.*(x86_64|amd64).deb'
- if: needs.v.outputs.leap-target != 'DEFAULT' && !(startsWith(needs.v.outputs.leap-target, "3") || startsWith(needs.v.outputs.leap-target, "4"))
name: Download Prev Leap Version
uses: AntelopeIO/asset-artifact-download-action@v3
with:
owner: AntelopeIO
repo: leap
target: '${{needs.v.outputs.leap-target}}'
prereleases: ${{fromJSON(needs.v.outputs.leap-prerelease)}}
file: 'leap.*amd64.deb' I guess PHBC is somewhat similar. Some particular platform may only be supported over a subset of all releases. But I think there is another tricky nuance here for these workflows too, including for 3.x & 4.0.. when we build Ultimately I'm not really sure what to do here.. |
@@ -290,7 +314,7 @@ jobs: | |||
|
|||
all-passing: | |||
name: All Required Tests Passed | |||
needs: [dev-package, tests, np-tests, libtester-tests] | |||
needs: [tests, np-tests, libtester-tests] |
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.
Removed dev-package
here since libtester-tests
job(s) already depend on it.
- cfg: {name: 'ubuntu20', base: 'ubuntu20', builddir: 'ubuntu20'} | ||
- cfg: {name: 'ubuntu22', base: 'ubuntu22', builddir: 'ubuntu22'} | ||
- cfg: {name: 'ubuntu20repro', base: 'ubuntu20', builddir: 'reproducible'} | ||
- cfg: {name: 'ubuntu22repro', base: 'ubuntu22', builddir: 'reproducible'} |
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 was not what I was expecting this to end up looking like; I was thinking there would be some sort of JSON file or such that defined the platform hierarchy for building and then testing. But simply ran out of time to explore other approaches. Though, honestly, other then copy pasting it 3 times this isn't too bad. But this is exactly the kind of plumbing this comment chain is discussing some, #1703 (comment)
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.
With care, this kind of copy/paste is OK, avoiding complicating logic.
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'd like to figure out a good way to avoid it, but that can come later. The problem I'm having is that any approach to avoid this repetition seems to introduce significant indirection.
For example, maybe I'd have a JSON or YAML file as part of the repo define the relationships between the platforms we're building, what that gets tested on, what runners are used (in case we ever add ARM etc), whether the libtester tests should run (on anything but ubuntu, no), etc. But that would involve a non-trivial chunk of, I guess, javascript at the start of the job to 'unpack' and process that definition in to various rules used in the workflow: complicating indeed.
Tracked by: #1717 |
cmake --build build -t package -- ${LEAP_BUILD_JOBS:+-j$LEAP_BUILD_JOBS} && \ | ||
src/tools/tweak-deb.sh build/leap_*.deb | ||
/__w/leap/leap/tools/tweak-deb.sh build/leap_*.deb |
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.
And in the perfect example for failure of,
It's important these two methods remain functionally identical for the produced .deb file to ensure that both CI & manual user builds produce the exact same output
This change was required for the above statement to remain true because in some cases full paths get baked in to the final executable. When built via build-base.yaml
the source path is effectively /__w/leap/leap/
while the reproducible.Dockerfile was /src/
. I entertained 3 ways to fix this,
- Change CI to place source in
/src
. This involves changes in many locations because weactions/checkout@v3
in many different jobs during the build & tests. - Change CI to make use of something like
-ffile-prefix-map=$PWD=/src
. This seems to work, but results in more variation of compile flags between CI & manual build. - Change reproducible.Dockerfile's build stage to place source in
/__w/leap/leap
like CI does.
Given the 11th hour, I opted for option 3 even though it has a negative trade off that CI only places source in /__w/leap/leap
if the repository is named leap
. If someone were to fork the repo to a different name, they would have divergence between CI & manual reproducible builds. But our current CI workflows are, unfortunately, ENF centric (see #416 for how this could be resolved; though I doubt something like the NP Tests will pass on a free GH runner these days), so this seems like an okay tradeoff for the time being. We can migrate to option 2 in the future is really needed.
I think I'm struggling a little with the definition of
Where I realistically think of the platform being used to build up the build or test environment as the Maybe there needs to be a distinction between the base platform to build up from and whether you're using reproducible in that context? |
I am not sure I understand, it seems like maybe the confusion is because some of the platforms are so overloaded? For example, the
which makes me wonder if we should overload them less, and have different |
That is kind of what I was getting at. In the matrix we have the option of defining things a little better like you did in the |
From the PR description:
Is there a way to codify this or test for it such that this one comment is not lost to history in the PR description and forgotten? |
The first hunch would be to add a script that is used in both locations to ensure they remain the same. But I find it rather unfortunate to add a layer of indirection for 3 or 4 lines. But there are some slight differences between the two, such as the manual user build ( Fortunately if they diverge the thought is we'll discover it via our code signing procedures: when the dev team goes to reproduce the build locally to sign it, we'll realize the CI and local builds don't match up. Now.. we haven't created this procedure yet, so I don't know exactly the steps -- it might even be manual verification; and it might be annoying late (right when we are about to do a release we discover a problem). Maybe not a good answer but it's something.
Yeah but that's the feature. The output from the reproducible build can run on many distros, and the 'reproducible platform' builds up the environment to accomplish that. I guess renaming the
What should the meaning of a platform file be? If we separate out the platforms to have a single role (even if that means some duplication and/or very simple platform files) does it become an improvement? |
Just as a follow-up to your suggestion above about a way to solve some of the issue in the near term, I used this over in another PR for a workflow requiring
It might be nice to find a way to have less code duplication by holding the file name in a variable that we could default to the new file: 'leap.*amd64.deb' and only overwrite that if the version was < 5 say up in the |
The script idea jumped out initially to me as well, but I think you pointed out the same issues with that as came to mind when I was debating it. Like you pointed out, maybe it isn't as big of an issue as it may seem initially, especially if engineers will most probably catch any differences during the signing procedure.
So for the two comment/response chains above -- I think I think we have a solution of getting around the naming conventions for downloading the correct artifacts, as we've discussed above. Should work for now and as things evolve, so can the solution. I am still curious though about how the |
To solve the obvious blockers, lets:
|
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.
Looks good to me.
- cfg: {name: 'ubuntu20', base: 'ubuntu20', builddir: 'ubuntu20'} | ||
- cfg: {name: 'ubuntu22', base: 'ubuntu22', builddir: 'ubuntu22'} | ||
- cfg: {name: 'ubuntu20repro', base: 'ubuntu20', builddir: 'reproducible'} | ||
- cfg: {name: 'ubuntu22repro', base: 'ubuntu22', builddir: 'reproducible'} |
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.
With care, this kind of copy/paste is OK, avoiding complicating logic.
Add the reproducible pinned build in to the main Build&Test Workflow (to be run on every PR etc). This build is then tested on both Ubuntu20 and Ubuntu22. The leap.deb file is stored as an artifact: this will be the binary file we release as a release asset.
The most shocking (and.. good) aspect of this workflow is that there is no problem taking the builddir from the Debian10+cmake3.27 environment, transporting it to a Ubuntu20+cmake3.16 & Ubuntu22+cmake3.22 environment, and all the tests continue to work correctly. I expected the difference in cmake versions at configure vs
ctest
time would cause some sort of problem which would necessitate the creation of a Ubuntu20+cmake3.27 & Ubuntu22+cmake3.27 platform to runctest
on instead. But this doesn't seem needed. We could still opt to take this approach if we wanted to be conservative and not mix up cmake/ctest versions.Something I want to be mindful of is commonality between what CI runs to do a reproducible build, and what an individual user performs to produce a reproducible build. What I want to avoid is a 2.0 type approach where CI & users nominally run 100% completely different scripts to perform the build. Certainly, ideally, the exact same steps are run in both cases. Unfortunately this doesn't quite get us to the perfect ideal. The good news is that both users and CI operate off the
builder
target intools/reproducible.Dockerfile
. But the final step in building Leap is a little different. Users will ultimately run thebuild
target intools/reproducible.Dockerfile
,leap/tools/reproducible.Dockerfile
Lines 101 to 103 in 0d56a95
But CI runs,
leap/.github/workflows/build_base.yaml
Lines 41 to 42 in 0d56a95
leap/.github/workflows/build.yaml
Lines 103 to 104 in 0d56a95
It's important these two methods remain functionally identical for the produced .deb file to ensure that both CI & manual user builds produce the exact same output.
work on #1641