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

Update docs #70

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Update docs #70

merged 1 commit into from
Jan 21, 2025

Conversation

schuellerf
Copy link
Contributor

@schuellerf schuellerf commented Jan 16, 2025

Followup of #68

without gpgme-devel neither
go run github.com/osbuild/image-builder-cli/cmd/image-builder@main (from the README.md)
nor
make build
work

(which essentially do the same)
So installing gpgme-devel makes sense, I guess, which is also documented in images and we include that here.

Makefile Outdated
@echo " scratch: Quick scratch build of RPM"
@echo " clean: Remove all built binaries"
@echo " lint: Run all known linters"
@awk 'match($$0, /^([a-zA-Z_\/-]+):.*? ## (.*)$$/, m) {printf " \033[36m%-30s\033[0m %s\n", m[1], m[2]}' $(MAKEFILE_LIST) | sort
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Print a sorted list of targets, I think?

Btw, this commit message seems to be incomplete:

Makefile: update help and improve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually not really, "improve…it"? :-D
I can rephrase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just prints the help from the comments - is used in many other repos to have the comment next to the target and the help always up to date ;)

Copy link
Member

Choose a reason for hiding this comment

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

actually not really, "improve…it"? :-D I can rephrase

in that case "Update and improve help" would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll wait for the general feedback which tags we use and how to unify them. Then I'll update the whole PR or maybe recreate it… a lot could change

README.md Outdated

## Prerequisites

Make sure to have the required `osbuild` RPMs installed:
Make sure to have the required `osbuild` and dependent RPMs installed:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a bit nitpicky, but I'm not sure calling it a "dependent RPM" is accurate here.

gpgme-devel is a dependency of image-builder-cli (via imports). It's not a dependent of osbuild or image-builder-cli (i.e. gpgme-devel does not depend on osbuild or image-builder-cli).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it seems to fit better under the "Complication" section as it is just a build-dependency, if one installs the rpm from copr this should not needed

Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks, some small inline suggestion

README.md Outdated

## Prerequisites

Make sure to have the required `osbuild` RPMs installed:
Make sure to have the required `osbuild` and dependent RPMs installed:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it seems to fit better under the "Complication" section as it is just a build-dependency, if one installs the rpm from copr this should not needed

README.md Outdated
@@ -32,12 +32,19 @@ $ go install github.com/osbuild/image-builder-cli/cmd/image-builder@main

We plan to provide rpm packages in fedora as well.

## Compilation

You can as well compile the application from local source with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its probably good to add comething like You can compile the application in cmd/image-builder with the normal go commandd (or spell out the commands). Just to avoid the impression that there is anything special needed that requires a makefile (those are very usual IME in go projects)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to document copy'n'paste ready commands and with the make command we can adapt it and do other related things like create a bin directory (uh which should be added to .gitignore, I just saw) and also test it with all the tags needed to build (with github actions)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but please also mention something like "You can compile the application in cmd/image-builder with the normal go command" just to avoid the impression that there is anything special needed that requires a makefile (those are very usual IME in go projects).

@schuellerf
Copy link
Contributor Author

@mvo5 I just found that there are multiple build commands scattered around, one which is in the Containerfile
go build -tags "containers_image_openpgp exclude_graphdriver_btrfs exclude_graphdriver_devicemapper" ./cmd/image-builder
and due to the containers_image_openpgp it does not need the gpgme library, although installed there 🤷‍♂️ …
(at least what I tested)
is this current PR still valid or should the make build also use those tags?
I'd love to unify those

As a dependency of `images` we also need `gpgme-devel` here,
if we compile without `containers_image_openpgp`.
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you

@mvo5 mvo5 added this pull request to the merge queue Jan 21, 2025
Merged via the queue into osbuild:main with commit f107665 Jan 21, 2025
19 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants