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.

Sign up for GitHub

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

Jump to bottom

cmd/build: add new --manifest-only, --manifest-path and -arch options #900

Closed
wants to merge 4 commits into from
Closed

cmd/build: add new --manifest-only, --manifest-path and -arch options #900

wants to merge 4 commits into from

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Sep 4, 2024

This PR tweaks the build binary to make it easier for otk to use it to generate reference images. With the tweaks here the tests in otk can generate reference images via:

$ go run github.com/osbuild/images/cmd/build@latest -type -distro centos-9 type qcow2 -arch aarch64 -manifest-only -manifest-path test/data/reference-images/<our-location>

that can then be used to validate that the otk generated images are identical to the ones generated by images

@mvo5
cmd/build: add new -manifest-only option
a8bcf64 
This option allows build to just generate the manifest but not
run `osbuild`. This is useful for the `otk` tests where a manifest
for a specific distro needs to get generated to produce identical
otk files.

Note that we also have `gen-manifests` but that seems geared towards
a different use-case so it's harder to use/change for this purpose.
@mvo5
cmd/build: make -config optional
0e30190 
The `build` binary is perfectly happy with an empty build config
so let's make this argument optional. Alternatively we could
validate and enforce non-empty build configs if there is actually
something in there that is important.
supakeen
supakeen reviewed Sep 4, 2024
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

The idea of having this is good but there's some confusion for me; how is this different from using cmd/gen-manifests. Perhaps it'd be better suited to add the manifest-path there?

@mvo5
Copy link
Contributor Author

mvo5 commented Sep 4, 2024 •
edited
Loading

The idea of having this is good but there's some confusion for me; how is this different from using cmd/gen-manifests. Perhaps it'd be better suited to add the manifest-path there?

Sure, that could also be done, I have no super strong opinion either way. My reasoning is that using gen-manifest seems to be more geared for the mass build task and it will be harder to add a "--manifest-path" as it will be auto-generated which means the other tool that drives it needs to know what the resulting filename is (or use the same pattern) and it also needs post-processing bcause AFAICT the generated files are not manifests but contain a build-request key and some extra data. Can all be done but I would rather move "build" into a more generally useful tool (that we can eventually use for project excosim) - but happy to hear other thoughts (e.g. @achilleas-k )

[edit: and maybe I'm missing something, this area I'm not super versed in]
[edit2: after thinking some more and talking to @achilleas-k I think this is maybe not needed, I will move to draft, it might be nice to have but it is no longer in the criticial path, I will use gen-manifests instead (thanks @supakeen )]

@mvo5
cmd/build: add new -manifest-path flag to select manifest json
09db08d 
This commit allows the user to select where the manifest json is
writen. This is especially useful in combination with
`--manifest-only` and is used by `otk` to generate reference images.
@mvo5
cmd/build: add -target-arch flag
95e39d3 
This flag allows to set the target architecture for the build. This
is most useful for `-manifest-only` and the intended use-case is
generating reference images for `otk`. But it could also be used
with `qemu-user` and should work equally well as `bib` (maybe
better as the "regular" distro code tends to use less of the
modern system calls that tend to be problematic like `openat2`).
@mvo5 mvo5 marked this pull request as draft September 4, 2024 14:13
@achilleas-k
Copy link
Member

achilleas-k commented Sep 5, 2024 •
edited
Loading

My reasoning is that using gen-manifest seems to be more geared for the mass build task and it will be harder to add a "--manifest-path" as it will be auto-generated which means the other tool that drives it needs to know what the resulting filename is (or use the same pattern)

This got me thinking if it would be useful to have a --filename-format flag that can interpret patterns. For example, our current (default) format would be (in standard Go template form):

{{ distro }}-{{ arch }}-{{ type }}-{{ config_name }}.json

(though we also do s/-/_/ on each component).

it also needs post-processing bcause AFAICT the generated files are not manifests but contain a build-request key and some extra data.

The extra bits can be skipped with --metadata=false in gen-manifests. That produces clean manifests.

EDIT: We could invert the default value for --metadata if it would make life easier.

@mvo5
Copy link
Contributor Author

mvo5 commented Sep 5, 2024

Closing as this is no longer used/needed for osbuild/otk#188

@mvo5 mvo5 closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supakeen supakeen supakeen left review comments

@achilleas-k achilleas-k Awaiting requested review from achilleas-k

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvo5 @achilleas-k @supakeen