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

document how to build under alpine #4353

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Jul 19, 2024

I had to build under alpine (musl), so I documented it. Hopefully this is helpful for others.

docs/alpine.md Outdated
To build under alpine linux, whether booted alpine or in a container:

```sh
apk add make gcc libseccomp-dev musl-dev linux-headers git
Copy link
Member

Choose a reason for hiding this comment

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

  1. Is ‘git’ really necessary?
  2. Even in the main Readme, there is no introduction to tell users to install gcc or musl, I think it’s the basic info that we should know. So if we want add some information about pre-requirements of build, the best place should be ‘../README.md’.
  3. This file is not referenced by any other files.
    WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah git is necessary. Go uses it under the covers as far as I know. I ran make runc and got an error for missing git.

Sure. Maybe this whole doc should be "prerequisites.md", cover both glibc and musl based OSes, and linked from README?

I'm more than happy to change this around as needed, as long as I know it will get accepted at the end.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

For my personal, I think we should add ‘pre-requirements.md’, because if we clone the repository to local for the first time, we usually can’t compile runc successfully in a big possibility, especially in GitHub online codespace. But we should consider other maintainers’ option.

Copy link
Member

@cyphar cyphar Jul 20, 2024

Choose a reason for hiding this comment

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

git isn't necessary for building. Our Makefile does use git to get the commit information to embed in the binary, but you can do make COMMIT="" to disable that. Go doesn't require git either (we vendor our dependencies, so you can build offline).

Also, the build will still work even with the warning -- $(shell ...) doesn't abort the build. Maybe we should silence errors for COMMIT ?= but that's a minor change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok. I didn't pay close enough attention to realize the deps had been vendored, so git would not be necessary.

Although, if the normal make path expects git, then I think listing git as a dependency makes sense. I can always add a comment about leaving it out if you do not include the commit as make COMMIT="".

In any case, what did we decide here? Do we want to make this a generic "prerequirements.md" or "build-requirements.md" or even just "building.md"/"build.md" and link from the main README.md?

Copy link
Member

Choose a reason for hiding this comment

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

@lifubang @cyphar wanna take another look at the new version?

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

@deitch
Copy link
Contributor Author

deitch commented Jul 22, 2024

OK, I merged all of this into the section in README that @AkihiroSuda recommended, added apt and yum commands for centOS, and moved the comment about optional seccomp to that section.

Ready for next run of review.

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

@deitch thanks for the PR! I've left some comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@deitch deitch force-pushed the alpine-build-docs branch 2 times, most recently from 419ef87 to e9ab2b4 Compare July 22, 2024 15:49
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

@deitch thanks a lot for the PR! LGTM

I've tested this in debian:testing-slim too, and it works fine.

Can you confirm that you tested it in all the distros you documented how it should compile?

README.md Show resolved Hide resolved
@deitch
Copy link
Contributor Author

deitch commented Jul 23, 2024

Yes correct. I ran each of these commands on the specific distro.

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@AkihiroSuda AkihiroSuda merged commit 459ce2f into opencontainers:main Jul 25, 2024
40 checks passed
@deitch deitch deleted the alpine-build-docs branch July 25, 2024 14:21
@lifubang lifubang mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants