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

feat: coreboot git describe #495

Merged
merged 5 commits into from
Jan 13, 2025
Merged

feat: coreboot git describe #495

merged 5 commits into from
Jan 13, 2025

Conversation

AtomicFS
Copy link
Collaborator

@AtomicFS AtomicFS commented Jan 8, 2025

The motivations behind this:

  • coreboot likes to embed its source version into the compiled binary
  • coreboot does that by calling git describe ...
  • however with firmware-action we can assume people will include coreboot as git submodule, and thus when compiling coreboot in container the git commands will fail because of missing .git directory
  • to fix this we will run manually the git describe command and then pass it in form of env variable into the container

The coreboot part crebootPassEnvVars is not tested now. I am too tired to look into unit-testing that right now (the supporting functions have unit tests).

I just ran the firmware-action on one project and tested it manually - it seems to work exactly as expected. Not sure if it is worth it to write unit-test.

@MDr164 if you think unit-test for the crebootPassEnvVars is a good idea, then I will make it tomorrow.

TODO:

  • Check for existence of .coreboot-version which should take precedence before auto-generated env var (should prevent creation of env var). Might be good idea to make a list of priorities in the comments.
  • I will also look into implementing this (read as copy-pasting) for Linux kernel too.

@AtomicFS AtomicFS requested a review from MDr164 as a code owner January 8, 2025 20:47
@github-actions github-actions bot added testing Testing related bugfix Bugfix / fix module/coreboot labels Jan 8, 2025
@AtomicFS AtomicFS enabled auto-merge January 8, 2025 20:47
@MDr164
Copy link
Collaborator

MDr164 commented Jan 8, 2025

Now the wrong Marv is even tagged on GitHub and not only on slack anymore, great 😁

@AtomicFS
Copy link
Collaborator Author

AtomicFS commented Jan 8, 2025

Oh, sorry, sleep deprivation :D

I will stop for today, probably doing more damage then anything else :D

@AtomicFS AtomicFS force-pushed the fix/coreboot-git-describe branch from 70450ac to 789fc2b Compare January 9, 2025 10:27
@AtomicFS
Copy link
Collaborator Author

AtomicFS commented Jan 9, 2025

@MDr164 btw, what do you think about the unit-testing?

The biggest hurdle I think would be the need for cbfstool, so that we can actually check the embedded version string.

The commands to check are:

cbfstool output-coreboot-RELEASE/coreboot.rom extract -n build_info -f /tmp/foo
grep COREBOOT_VERSION /tmp/foo

@AtomicFS AtomicFS force-pushed the fix/coreboot-git-describe branch 2 times, most recently from feb4f73 to 3ed5c8e Compare January 9, 2025 12:49
@MDr164
Copy link
Collaborator

MDr164 commented Jan 9, 2025

btw, what do you think about the unit-testing?

Gotta take a closer look later, sadly I'm a bit busy this week

The biggest hurdle I think would be the need for cbfstool, so that we can actually check the embedded version string.

That is part of coreboot and could be precompiled inside the container if we need it. Some distributions even package it already.

The commands to check are:

cbfstool output-coreboot-RELEASE/coreboot.rom extract -n build_info -f /tmp/foo
grep COREBOOT_VERSION /tmp/foo

Doesn't look too hard and could be automated, no?

@AtomicFS
Copy link
Collaborator Author

AtomicFS commented Jan 9, 2025

btw, what do you think about the unit-testing?

Gotta take a closer look later, sadly I'm a bit busy this week

No problem ;)

The biggest hurdle I think would be the need for cbfstool, so that we can actually check the embedded version string.

That is part of coreboot and could be precompiled inside the container if we need it. Some distributions even package it already.

Ou yeah, you are correct - we are actually running some tests inside the Docker containers. Therefore this is easy to implement! I have completely forgot about that :D

I thought we would have to implement testing inside containers :D Thanks past me for so good coverage!

The commands to check are:

cbfstool output-coreboot-RELEASE/coreboot.rom extract -n build_info -f /tmp/foo
grep COREBOOT_VERSION /tmp/foo

Doesn't look too hard and could be automated, no?

Yeah, if we execute the test inside Docker container, it is trivial. So let's do it.

@AtomicFS AtomicFS force-pushed the fix/coreboot-git-describe branch from 3ed5c8e to fb131cf Compare January 9, 2025 15:53
- the motivations behind this:
- coreboot likes to embed its source version into the compiled binary
- coreboot does that by calling 'git describe ...'
- however with firmware-action we can assume people will include
  coreboot as git submodule, and thus when compiling coreboot in
  container the git commands will fail because of missing .git directory
- to fix this we will run manually the 'git describe' command and then
  pass it in form of env variable into the container

Signed-off-by: AtomicFS <[email protected]>
Signed-off-by: AtomicFS <[email protected]>
@AtomicFS AtomicFS force-pushed the fix/coreboot-git-describe branch from fb131cf to ab73ad3 Compare January 9, 2025 16:25
@AtomicFS AtomicFS added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit 990ce12 Jan 13, 2025
68 checks passed
@AtomicFS AtomicFS deleted the fix/coreboot-git-describe branch January 13, 2025 10:12
@AtomicFS
Copy link
Collaborator Author

I was working on the unit test :D

But I guess we can merge that in separate PR :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bugfix / fix module/coreboot testing Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants