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(cmd): change BuildCommand into slice #519

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AtomicFS
Copy link
Collaborator

@AtomicFS AtomicFS commented Jan 23, 2025

  • this is a breaking change
  • this should make the JSON configuration files much more readable for edk2
  • this change means we have to drop DefconfigPath for edk2

fixes #518

@AtomicFS AtomicFS requested a review from MDr164 as a code owner January 23, 2025 10:32
@AtomicFS AtomicFS enabled auto-merge January 23, 2025 10:32
@github-actions github-actions bot added feature New feature or request testing Testing related module/edk2 labels Jan 23, 2025
@AtomicFS AtomicFS force-pushed the feat/edk2-build-command-into-slice branch from 9558e59 to 72b9ef7 Compare January 23, 2025 10:50
@AtomicFS
Copy link
Collaborator Author

AtomicFS commented Jan 23, 2025

Failing because of

error:
input:
container.from resolve:
failed to resolve image ghcr.io/9elements/firmware-action/coreboot_4.19:main:
failed to resolve source metadata for ghcr.io/9elements/firmware-action/coreboot_4.19:main:
failed to copy:
httpReadSeeker:
failed open:
content at https://ghcr.io/v2/9elements/firmware-action/coreboot_4.19/manifests/sha256:b0beef78d1d9a2398e5909c6ccb92416eccb1aa7fc33e60ea442493b0de14d00 not found:
not found

- this is a breaking change
- this should make the JSON configuration files much more readable for
  edk2
- this change means we have to drop DefconfigPath for edk2

Signed-off-by: AtomicFS <[email protected]>
@AtomicFS AtomicFS force-pushed the feat/edk2-build-command-into-slice branch from 72b9ef7 to 6754103 Compare January 24, 2025 09:30
@AtomicFS AtomicFS disabled auto-merge January 27, 2025 09:41
@AtomicFS
Copy link
Collaborator Author

Now when I am thinking about this, it is inconvenient that stuff like source or exported variable will not work across elements in the slice. Maybe on each step we could export the env and the import it before the next command?

I don't know ... not sure if this is a good idea anymore.

Copy link
Collaborator

@MDr164 MDr164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the removed DefconfigPath for EDK2 is now part of the BuildCommand field or how does that work in practice?

@AtomicFS
Copy link
Collaborator Author

So the removed DefconfigPath for EDK2 is now part of the BuildCommand field or how does that work in practice?

I did not know in which order the PR's will get merged ;)

@AtomicFS
Copy link
Collaborator Author

AtomicFS commented Jan 27, 2025

Oh sorry, I misread the comment. Yes, the general idea is to not use the DefconfigPath for edk2. It was Patrick's idea if I am not mistaken. It is to store build flags and arguments for edk2 build. Very hacky non-standard solution.

I believe that it was meant to simplify local builds.

If we would switch to BuildCommand as slice, then the whole JSON config would be more readable and then I would not see any good reason for this DefconfigPath.

However the issue is that exported variables or sourceed files will not work across elements in the slice. But having the commands separately would be much nicer for debugging with interactive dagger debug, I think.

@AtomicFS AtomicFS added the blocked Blocked by another label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another feature New feature or request module/edk2 testing Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor edk2 BuildCommand into slice of strings
2 participants