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

libct: internalize some packages #3049

Closed
wants to merge 4 commits into from

Conversation

kolyshkin
Copy link
Contributor

This moves a few packages from libcontainer to internal, so that they are no longer available to external users. This is the first step in implementing #3028.

I chose internal over, say, libcontainer/internal or pkg/internal merely because of a short path.

@kolyshkin
Copy link
Contributor Author

Hope this won't conflict with #3033 as it will be a nightmare to rebase that one.

AkihiroSuda
AkihiroSuda previously approved these changes Jun 29, 2021
kolyshkin added 4 commits July 1, 2021 11:06
The package has no external users (at least according to
https://pkg.go.dev/github.com/opencontainers/runc/libcontainer/capabilities?tab=importedby)
and thus is a good candidate for internalization.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

Are there any objections or comments about this PR, @opencontainers/runc-maintainers? I have another PR (#3056) that is based on this one and want to move forward with it.

@hqhq
Copy link
Contributor

hqhq commented Jul 14, 2021

I personally prefer libcontainer/internal, because libcontainer is supposed to be the library of container, even these files are moved to internal, they should still be part of the "library of container".

@kolyshkin
Copy link
Contributor Author

I personally prefer libcontainer/internal, because libcontainer is supposed to be the library of container, even these files are moved to internal, they should still be part of the "library of container".

First, I was hoping to make those import paths shorter (or at least not longer).

Second, runc/libcontainer is used by other projects, while runc/internal clearly contains a set of packages aimed for runc itself. Are those packages about containers? Yes, they are -- the whole runc is, and no, they aren't -- say, capabilities package is about process caps, and is not really about containers per se.

I'd like to keep it as is (mostly because it's shorter).

@hqhq
Copy link
Contributor

hqhq commented Jul 21, 2021

I personally prefer libcontainer/internal, because libcontainer is supposed to be the library of container, even these files are moved to internal, they should still be part of the "library of container".

First, I was hoping to make those import paths shorter (or at least not longer).

Second, runc/libcontainer is used by other projects, while runc/internal clearly contains a set of packages aimed for runc itself. Are those packages about containers? Yes, they are -- the whole runc is, and no, they aren't -- say, capabilities package is about process caps, and is not really about containers per se.

Yes, everything in runc repo is about container, but the phrase I used was library of container,
it's different with just container.

Back to the old days, we only have libcontainer, then we moved it to a separate repo to build runc based
on it, runc only added very limited things, the most important part is the cli, other than that, just minor
things like scripts, test cases, docs, man pages, etc.

According to #3028 , we are about to move most of libcontainer packages to internal, which means we are
removing the ability to create containers just by importing libcontainer and using its APIs, it's fine to me if
nobody is doing so. But I think we can still keep libcontainer as independent, it's kind of the core of container
runtime.

I'm happy to change my mind if there are other concerns, just shorter path isn't a compelling reason for me.
@cyphar @crosbymichael WDYT?

I'd like to keep it as is (mostly because it's shorter).

@kolyshkin kolyshkin marked this pull request as draft July 28, 2021 03:22
@kolyshkin
Copy link
Contributor Author

draft until we decide on the path.

@kolyshkin
Copy link
Contributor Author

No consensus on the path for internal packages, so I'm closing this one.

@kolyshkin kolyshkin closed this Aug 24, 2021
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.

3 participants