Ds08msa internal v0.4 #619
Conversation
|
@miczyg1 @SergiiDmytruk I get merge conflict because of What is our policy in such a situation? In theory, this PR should not be reviewed because of a merge conflict. EDIT: now I see: |
I think all boards are updated at the same time largely because not doing it can cause a board to fail building as a result EDK2-related changes in coreboot.
Tag will use an older revision anyway, so I don't really see a problem: tag uses tested EDK, |
Understood. I probably should better split this PR between what I relay on for training vs what could be useful for others. Obviously my work cannot be proven by reproducibility because we don't build timeless and hash of current commit is taken into build. So what I'm really looking is advise how this PR should be submitted, because first I should not request review if I have merge request (we know how it would end), and second I would like not to broke other uses cases. AFAIK we don't have any tests to confirm if this script keep working as expected.
You're right somehow I though that was hash before rebase, but it was my mistake instead of hash which disapeared due to rebase. Sorry for the noise. |
Why? If we build from the same commit using the same SDK, it should give the same result. Why should we care if N code revisions/commits give the same hash or not? To me a different commit means different code and shouldn't give the same hash. |
I'd just rebase this branch onto |
For years, I've disliked coreboot-sdk and how it impacts the whole building story in the long run, including the reproducibility of older builds. We essentially can't achieve this with the coreboot build environment: https://reproducible-builds.org/docs/perimeter/ For various reasons, I cannot build from the same SDK:
For various reasons, I may have issues with building from the same commit - dasharo-pq. The code here makes no change to Odroid-H4+. I changed only the toolchain (not really since I still rely on the same compiler from coreboot-sdk Maybe this is not important to you; maybe my use cases are niche and unnecessary, but this PR contains a couple of things that help me:
Some of those features have generic value for the whole project, so I'm asking whether and how I could contribute to those changes. Or, more directly, if needed, how to split this PR so some of those required features can be merged without affecting everything else (hence the lack of testability in this area of code?). EDIT: I edited airgap requirement |
|
@pietrushnic This script build.sh is for mere mortals, not devs. It is supposed to make building simple and robust. That is why:
And how BUILD_TIMELESS helps if you change the SDK in build.sh? It will produce a different binary from the same code, because the toolchain was different, but it went undetected, because of BUILD_TIMELESS. Building with build.sh and without BUILD_TIMELESS ensures that everybody build the binaries the same way we do. We also avoid entering a support hell where everybody has a different way of building binaries. |
Fair point. I can do that modification. I can make it transparent for the user by:
Is that acceptable?
To clarify:
To the point how BUILD_TIMELESS helps, lack of it does not help:
We could also consider how the source version control system is part of the build environment. It is no longer part of the build environment source code that should be able to exist with or without SVC. The fact that the build environment is mangled with SVC is a terrible design, leading to issues described above, such as the same code with the same compiler giving you different build results. Please consider how crazy it is that zero change in code gives different build results because even git amend with no change to code would lead to a change in build result hash. This is very fragile. I understand this can be out of the scope of the build.sh, and that is fine. I also know that those problems may not be important to Dasharo developers with different issues. I'm building awareness and a path for my use cases, not being the enemy of the primary use case but being able to coexist.
We already have situations where not everyone builds the binaries similarly, but let's leave it alone. You are correct that this is most common and adopted for most Dasharo-supported hardware build processes. We can also agree that if it doesn't work for me, it is my problem, so Dasharo developers should not get more work from my use cases (training, dasharo-pq). That's why I'm asking for feedback on how to achieve that in a non-interfered way. I could create my build.sh? I need that solution upstream because I build the whole infrastructure on that (PET and dasharo-pq). |
It would simply need
Yes.
Not in this particular case, but I was saying rather hypothetically.
I'm not an GCC expert, but newer GCC version may have different features, optimization, security fixes which may impact how final binary looks like after compilation. That's what I am trying to say.
I can't agree with
Why? You can do BUILD_TIMELESS using any toolchain you want and have that binary as a reference, then do whatever you want with toolchain, use BUILD_TIMELESS again and compare the results. You are not forced to use build.sh, manually invoking commands will work too. As simple as that.
But at least the BIOS release date in SMBIOS is correct :)
Yes, not everyone builds the binaries the same way. But we can conclude it with one sentence "please use build.sh" and the end of topic. If you need the BUILD_TIMELESS, then add it to the script or make the script pass the environments BUILD_TIMELESS to the docker container. I'm fine with that. However, no matter what we do, we will not fix the past, i.e. no way to get the same hash as release binary if we modify anything in the tree (build.sh including). Thus as I proposed earlier, you can always build the reference with BUILD_TIMELESS by manually pasting commands. Then re-check against this reference after any modifications you make. |
Thanks. I will add it.
This was probably tested with the QEMU target with coreboot. Toolchain often breaks with our EDKII, which is mainly our issue, not upstream. If iPXE breaks, then upstream fix that as part of the bump of iPXE. Not sure about other payloads. However, this proves that the toolchain has a problem because it has to build a coreboot and many different projects. As I said, this is not an argument, so there is no need to rebut.
Of course, I ended up with more work on explaining all the necessary details to students and dealing with the support of it live during the class. Also, my CI/CD related to dasharo-pq and PET have to be coded differently than what is used by most developers. I don't say it is not possible. It is, but I have to give up the infrastructure used by all other developers and do some more work. Before that, I wanted to try this path. I understand you say BUILD_TIMELESS is not a good fit for build.sh. That is fine. I will try to find another way. I have a gut feeling this discussion will get back to us sooner or later in another form, but maybe right now, let's give it up.
Yes, I will follow both:
|
pietrushnic
force-pushed
the
ds08msa_internal_v0.4
branch
3 times, most recently
from
c0b1a7a to
f902cda
Compare
pietrushnic
force-pushed
the
ds08msa_internal_v0.4
branch
from
463d406 to
14dfd52
Compare
Signed-off-by: Piotr Król <[email protected]>
pietrushnic
force-pushed
the
ds08msa_internal_v0.4
branch
from
14dfd52 to
9d6d9c2
Compare
|
@miczyg1 I simplified the whole change into three things:
Please let me know if this is better and match Dasharo needs. |
This change look for AIRGAP environment variable and if it is set it
perform airgap build of Dasharo for Odroid H4 and its version for
Intel Boot Guard.
This is required for security, privacy and trainers who would like to
perform 100% offline build.
To make that possible couple requirements have to be fulfilled:
- repository cannot be distcleaned, because it remove all artifacts, the
assumption is that provided repository already has all dependencies
fetched, so only make clean is made before proceeding
- since whole process rely on mounting edk2 as volume inside Dasharo SDK
container, workspace directory to which it would be mount needs proper
permissions otherwise docker will create mountpoint with root
privileges, what cause issues in further use and build process
- finally we take into consideration BUILD_TIMELESS environment
variable, which improve testability of build process and toolchain
change
This change was tested by:
1. cloning relevant version of edk2
2. cloning coreboot, cd coreboot
3. running checkout on ipxe:
docker run --rm --user $(id -u):$(id -g) -v $PWD:/home/coreboot/coreboot \
${DASHARO_SDK} \
make -C /home/coreboot/coreboot/payloads/external/iPXE checkout
4. Build
EDK2_REPO_PATH="${PWD}/../edk2" AIRGAP=1 BUILD_TIMELESS=1 ./build.sh odroid_h4_btg
EDK2_REPO_PATH="${PWD}/../edk2" AIRGAP=1 BUILD_TIMELESS=1 ./build.sh odroid_h4
Signed-off-by: Piotr Król <[email protected]>
Signed-off-by: Piotr Król <[email protected]>
pietrushnic
force-pushed
the
ds08msa_internal_v0.4
branch
from
bb1b9a0 to
1e016c4
Compare
We cannot use
|
| @@ -30,7 +30,8 @@ usage() { | |||
| } | |||
|
|
|||
| SDKVER="2024-02-18_732134932b" | |||
|
|
|||
| DASHARO_SDK=${DASHARO_SDK:-"ghcr.io/dasharo/dasharo-sdk:latest"} | |||
There was a problem hiding this comment.
Should point to a stable tag?
There was a problem hiding this comment.
So I assume we can merge and make v1.6.0 release Dasharo/dasharo-sdk#11 ?
There was a problem hiding this comment.
Yes, it has been approved long time ago. Feel free to merge and tag it @pietrushnic
| -v $HOME/.ssh:/home/coreboot/.ssh \ | ||
| -w /home/coreboot/coreboot coreboot/coreboot-sdk:$SDKVER \ | ||
| /bin/bash -c "make distclean" | ||
| if [ "${AIRGAP}" -eq 1 ]; then |
There was a problem hiding this comment.
Should be applied to all boards, not just selected ones. Mayeb it would also be worth to separate it into a function to reduce the LOC and overall boilerplate.
| if [ "${AIRGAP}" -eq 1 ]; then | ||
|
|
||
| # In this situation we assume that provided repository is ready to be used | ||
| # and nothing should be downloaded during build process. | ||
|
|
||
| if [ -d "${EDK2_REPO_PATH}" ]; then | ||
| # Without following sequence workspce would be created by docker with root | ||
| # privilidges and build will fail. | ||
| # Target directory | ||
| TARGET_DIR="payloads/external/edk2/workspace/Dasharo" | ||
| mkdir -p "$TARGET_DIR" | ||
| chown -R $(id -u):$(id -g) "$TARGET_DIR" | ||
| chmod -R 755 "$TARGET_DIR" | ||
| docker run --rm -t -u $UID -v $PWD:/home/coreboot/coreboot \ | ||
| -v $HOME/.ssh:/home/coreboot/.ssh \ | ||
| --network none \ | ||
| ${EDK2_REPO_PATH:+-v $EDK2_REPO_PATH:/home/coreboot/coreboot/payloads/external/edk2/workspace/Dasharo} \ | ||
| -e BUILD_TIMELESS=${BUILD_TIMELESS} \ | ||
| -w /home/coreboot/coreboot ${DASHARO_SDK} \ | ||
| /bin/bash -c "make olddefconfig && make -j$(nproc)" | ||
| else | ||
| echo "EDK2_REPO_PATH is not defined in AIRGAP!" | ||
| exit 1 | ||
| fi | ||
| else | ||
| docker run --rm -t -u $UID -v $PWD:/home/coreboot/coreboot \ | ||
| -v $HOME/.ssh:/home/coreboot/.ssh \ | ||
| -w /home/coreboot/coreboot ${DASHARO_SDK} \ | ||
| /bin/bash -c "make olddefconfig && make -j$(nproc)" | ||
| fi |
There was a problem hiding this comment.
Should be applied to all boards, not just selected ones. Maybe it would also be worth to separate it into a function to reduce the LOC and overall boilerplate. I think the docker commands to start the build are the same for every board.
|
@pietrushnic since you change the SDK in build.sh, it would be wise to change it in GH action workflows as well. That way CI will build test everything using your new SDK |
Rebased and moved to new branch name from #573
We have to decide what changes here make sense. I already wrote a justification for the use of Dasharo SDK here: Dasharo/dasharo-sdk#9
In the long run, I hope we can replace dasharo-sdk with something like stageX.