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

libcontainer: configs: ensure can build on darwin #3697

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

egernst
Copy link
Contributor

@egernst egernst commented Jan 11, 2023

configs package can no longer be built on non-Linux OS, such as Darwin.

When running GOOS=darwin go build on the packge, we had the following
errors:

./configs/mount.go:34:16: undefined: unix.MountAttr
./configs/mount.go:47:22: undefined: unix.MS_BIND

Let's ensure that the linux specific bits are handled in mount_linux.go,
and introduce a _unsupported file, similar to how cgroups file is
handled within the package. This'll facilitate utilization of the pkg
for other projects that care about Darwin.

Signed-off-by: Eric Ernst eric_ernst@apple.com

@egernst
Copy link
Contributor Author

egernst commented Jan 11, 2023

AFAICT, these CI failures aren't introduced by my change. noop PR also failing: #3698

@egernst
Copy link
Contributor Author

egernst commented Jan 11, 2023

/cc @AkihiroSuda, @alban @rata

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.

It would be great if you can paste the compilation error you see (not all the lines, just the important ones if it is too verbose), so we can easily know how this fails and why this fixes it.

With the info you posted (here and in the issue) is hard to know if some changes are needed (posted those inline).

Also, why do you need this to compile on Mac? I mean, none of the cgroup stuff works there, right? Is it even a supported platform for runc?

I'm fine with the PR, but if we don't want to regress, we should probably add a test to compile on Mac, that I'm not sure what maintainers think about that.

libcontainer/configs/mount_linux.go Show resolved Hide resolved
libcontainer/configs/mount.go Show resolved Hide resolved
libcontainer/configs/mount_unsupported.go Show resolved Hide resolved
@egernst
Copy link
Contributor Author

egernst commented Jan 12, 2023

It would be great if you can paste the compilation error you see (not all the lines, just the important ones if it is too verbose), so we can easily know how this fails and why this fixes it.

Good call - updated the PR description

Also, why do you need this to compile on Mac? I mean, none of the cgroup stuff works there, right? Is it even a supported platform for runc?

Libcontainer configs is utilized in other projects, including Kata Containers, which is adding support for building and running on Darwin.

I'm fine with the PR, but if we don't want to regress, we should probably add a test to compile on Mac, that I'm not sure what maintainers think about that.

Sure - you don't need to be on Mac. Just run "GOOS=darwin go build".

@rata
Copy link
Member

rata commented Jan 12, 2023

Also, why do you need this to compile on Mac? I mean, none of the cgroup stuff works there, right? Is it even a supported platform for runc?

Libcontainer configs is utilized in other projects, including Kata Containers, which is adding support for building and running on Darwin.

Ohh, seems reasonable. Can you add that to the PR description and commit message?

I'm fine with the PR, but if we don't want to regress, we should probably add a test to compile on Mac, that I'm not sure what maintainers think about that.

Sure - you don't need to be on Mac. Just run "GOOS=darwin go build".

Right, thanks! :)

@egernst egernst force-pushed the fixup-configs branch 2 times, most recently from 9fb5f34 to bb6bff0 Compare January 12, 2023 17:33
@egernst
Copy link
Contributor Author

egernst commented Jan 12, 2023

Updated the commit/PR to add more detail, and made the changes to dedup the struct based on your feedback. Thanks Rodrigo!

rata
rata previously approved these changes Jan 12, 2023
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.

LGTM, thanks!

libcontainer/configs/mount.go Outdated Show resolved Hide resolved
rata
rata previously approved these changes Jan 12, 2023
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.

LGTM

@rata
Copy link
Member

rata commented Jan 12, 2023

Ups, the linter seems very unhappy about this change. Can you fix it? :)

@egernst
Copy link
Contributor Author

egernst commented Jan 12, 2023

Ouch, this deals less with lint, and more with compilation failure. My bad.

The issue is not being able to directly compose a struct that has an embedded struct (see golang/go#9859). As an example, we cannot directly initialize the embedded structs fields:

			m := &configs.Mount{Destination: node.Path, Source: node.Path}

This results in the Field Destination doesn't exist. We would need to do something like:

			m := &configs.Mount{}
			m.Destination = node.Path
			m.Source = node.Path

I don't love the making that change throughout the code - the UX in Golang for this is quite lacking.

Curious to hear feedback on how you think it'd make sense to proceed here (ie, dup the struct or make changes in code base to expose the embedded struct).

For me, I think just duplicating those fields, while not ideal, makes more sense. Pushing a commit to remove the embedded struct for your feedback.

configs package can no longer be built on non-Linux OS, such as Darwin.

When running `GOOS=darwin go build` on the packge, we had the following
errors:
```
./configs/mount.go:34:16: undefined: unix.MountAttr
./configs/mount.go:47:22: undefined: unix.MS_BIND
```

Let's ensure that the linux specific bits are handled in mount_linux.go,
and introduce a _unsupported file, similar to how cgroups file is
handled within the package. This'll facilitate utilization of the pkg
for other projects that care about Darwin.

Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Copy link

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM, thanks!

@AkihiroSuda AkihiroSuda merged commit ba994dc into opencontainers:main Jan 19, 2023
@egernst egernst deleted the fixup-configs branch January 19, 2023 04:46
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.

5 participants