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

Add FreeBSD support #357

Merged
merged 3 commits into from Mar 11, 2021
Merged

Add FreeBSD support #357

merged 3 commits into from Mar 11, 2021

Conversation

mateuszkwiatkowski
Copy link
Contributor

@mateuszkwiatkowski mateuszkwiatkowski commented Feb 2, 2021 •
edited

This PR add support for building umoci on FreeBSD.

Ref. #330

@cyphar
Copy link
Member

cyphar commented Feb 4, 2021

You need to add a Signed-off-by: line to your commit(s) which indicates that you attest the Developer Certificate of Origin a statement about your contributions that you must read before signing (don't worry, it's quite short and easy-to-read). You can add it to your commits with git commit --amend -s, and then doing a git push --force.

NOTE: This is a saved reply. Sorry if it reads as a cookie-cutter response, it was written so that newcomers understand what a "DCO" is and make the process for contributing a little less scary.

@mateuszkwiatkowski mateuszkwiatkowski force-pushed the support-freebsd branch 3 times, most recently from 411bebf to a5afd68 Compare February 4, 2021 12:36
@mateuszkwiatkowski
Copy link
Contributor Author

@cyphar thank you for a tip. It should be fine now.

@mateuszkwiatkowski
Copy link
Contributor Author

Hey @cyphar,
Did you have a chance to take a look? Is it good direction to introduce FreeBSD support? I’ll be happy to work further on this, I just need your feedback. 👨🏻‍🔧

@mateuszkwiatkowski
Copy link
Contributor Author

@cyphar friendly reminder about this PR. :-)

cyphar
cyphar requested changes Feb 23, 2021
View reviewed changes
pkg/fseval/mknod_freebsd.go Outdated Show resolved Hide resolved
@mateuszkwiatkowski
Add vim swap file to .gitignore
4a16dca 
Signed-off-by: Mateusz Kwiatkowski <mateusz@serveraptor.com>
@mateuszkwiatkowski mateuszkwiatkowski force-pushed the support-freebsd branch 3 times, most recently from 53eb426 to 92bf36c Compare February 26, 2021 09:19
@mateuszkwiatkowski
Make Makefile more portable
6fbd32e 
Signed-off-by: Mateusz Kwiatkowski <mateusz@serveraptor.com>
cyphar
cyphar requested changes Feb 26, 2021
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

A couple of issues with types and a minor cleanup to avoid duplicating Tarmode.

pkg/system/mknod_linux.go Outdated Show resolved Hide resolved
pkg/system/mknod_linux_test.go Outdated Show resolved Hide resolved
pkg/system/mknod_linux.go Outdated Show resolved Hide resolved
pkg/fseval/fseval_default.go Outdated Show resolved Hide resolved
oci/layer/generate_linux_test.go Outdated Show resolved Hide resolved
oci/layer/generate_linux_test.go Outdated Show resolved Hide resolved
oci/layer/tar_extract_linux_test.go Outdated Show resolved Hide resolved
@mateuszkwiatkowski
Copy link
Contributor Author

@cyphar thank you for your patience. I addressed your feedback again.

@cyphar
Copy link
Member

cyphar commented Feb 26, 2021

If you can just squish your last three commits into one (so that the mknod changes are all in one commit), and CI passes this should be a good to go. Thanks! :D

@mateuszkwiatkowski
Copy link
Contributor Author

@cyphar squashed and force-pushed!

@mateuszkwiatkowski
Copy link
Contributor Author

One more commit incomming to fix failed test.

@mateuszkwiatkowski
Copy link
Contributor Author

mateuszkwiatkowski commented Feb 26, 2021 •
edited

Ok, this one surprised me:

docker run --rm -v /home/travis/gopath/src/github.com/opencontainers/umoci:/go/src/github.com/opencontainers/umoci --security-opt apparmor=unconfined --security-opt label=disable --security-opt seccomp=unconfined --security-opt systempaths=unconfined  -e CODECOV_ENV -e CODECOV_TOKEN -e CODECOV_URL -e CODECOV_SLUG -e VCS_COMMIT_ID -e VCS_BRANCH_NAME -e VCS_PULL_REQUEST -e VCS_SLUG -e VCS_TAG -e CI_BUILD_URL -e CI_BUILD_ID -e CI_JOB_ID -e CI -e TRAVIS -e SHIPPABLE -e TRAVIS_BRANCH -e TRAVIS_COMMIT -e TRAVIS_JOB_NUMBER -e TRAVIS_PULL_REQUEST -e TRAVIS_JOB_ID -e TRAVIS_REPO_SLUG -e TRAVIS_TAG -e TRAVIS_OS_NAME  --privileged --cap-add=SYS_ADMIN -e COVERAGE -e TESTS umoci/ci:latest make local-test-integration
623TESTS="" hack/test-integration.sh
624make: which: Command not found

I think it's from SHELL := $(shell which bash) in Makefile.
I'll try to figure out a bit later.

Edit: This docker run worked for me locally.

@mateuszkwiatkowski mateuszkwiatkowski force-pushed the support-freebsd branch 3 times, most recently from 5a77e6a to b158474 Compare March 4, 2021 08:06
@mateuszkwiatkowski
Provide system dependend wrapper for unix.Mknod
ea32e1d 
FreeBSD supports 64bit inodes so it requires that dev provided to
unix.Mknod is type uint64. This commit creates wrapper for unix.Mknod
to fix this issue.

Signed-off-by: Mateusz Kwiatkowski <mateusz@serveraptor.com>
@mateuszkwiatkowski
Copy link
Contributor Author

Hi @cyphar,

Few more fixes and CI is happy. Changes:

  • mknod_linux.go to mknod_unix.go to fix build on Darwin
  • Tarmode() deserved separate file to avoid duplication
  • added which package to fix make in Docker

cyphar
cyphar approved these changes Mar 6, 2021
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment •
edited

Choose a reason for hiding this comment

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

LGTM. I will look at adding a CI check to make sure we don't use unix.Mknod by accident (as well as adding FreeBSD to our CI checks) -- thanks for the PR!

@cyphar
Copy link
Member

cyphar commented Mar 6, 2021

/cc @tych0

@mateuszkwiatkowski
Copy link
Contributor Author

@cyphar thank you for your time. Is lack of that test a blocker for this PR or can we have it merged and add that test later?
Regarding the CI platform, I think that https://cirrus-ci.org is preferred by some FreeBSD native projects.

@cyphar
Copy link
Member

cyphar commented Mar 8, 2021

No, it's not a blocker. I'm waiting for @tych0 to review this since we need two LGTMs.

@mateuszkwiatkowski
Copy link
Contributor Author

FYI: I've just created and pushed freebsd image to Docker Hub: https://hub.docker.com/r/kwiat/freebsd/tags

@tych0
Copy link
Member

tych0 commented Mar 8, 2021

LGTM.

@mateuszkwiatkowski
Copy link
Contributor Author

@cyphar @tych0 could you please merge this PR? I have a port for FreeBSD Ports ready for submission but it requires that the change is in master and new release is tagged. Thank you in advance.

@cyphar
Copy link
Member

cyphar commented Mar 11, 2021

Yup, I think Tycho forgot to hit the merge button.

@cyphar cyphar closed this in 5412ded Mar 11, 2021
@cyphar cyphar merged commit 5412ded into opencontainers:master Mar 11, 2021
@mateuszkwiatkowski
Copy link
Contributor Author

@cyphar woohoo! thank you for cooperation and your assistance. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyphar cyphar cyphar approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@mateuszkwiatkowski @cyphar @tych0