pkg/cdi: remove github.com/opencontainers/runc/libcontainer dependency #158
Conversation
|
@elezar PTAL 🤗 |
|
Kicked off the tests; looks sane to me, and removing dependencies is always great. @thaJeztah Please check the failing tests, the PR looks incomplete regarding the go packages. |
|
oh! hm.. interesting; let me check |
thaJeztah
force-pushed
the
no_libcontainer
branch
from
5600187 to
8fe207b
Compare
|
Ah! only |
|
I'm afk for a bit, but will look on Wednesday when I'm back at my desk. The failures in binary builds are probably due to the modules for the cmds needing updating. There are some vendor make targets that should do this. |
|
@elezar I will make sure the tests work and do a first pass, you can add your review when you're back. |
|
@thaJeztah |
|
Can you run the tests on your machine first? |
|
Ah, dang; I know what's happening; the type differs between platforms (macOS uses different type than other Unix'es); will fix in a bit. |
thaJeztah
force-pushed
the
no_libcontainer
branch
from
8fe207b to
15f6c80
Compare
|
ok; changed it back to a switch, and ran tests in a container; I think it should be happy now |
|
Hm... DCO check won't kick in for some reason 🤔 |
There was a problem hiding this comment.
@thaJeztah thanks for the cleanup. Removing the runc dependency here is great!
I think the DCO failure may be due to the migration of repos. Let me investigate.
If part of that was to mark the DCO check as "required", then I think you may be running into a bug in the DCO check. What I ran into with that utility, is that it doesn't like marking it as a required check if there are existing pull requests in the repository; in that case it will never start. A workaround for that is to make it temporarily "optional", and mark it "required" again when there are no open pull request (but, erm.. yeah, that's a bit of a pain on active repositories) |
I unfortunately don't have permissions to add / edit the DCO check in this repository, so may have to bypass this for now. I have confirmed that your commit is signed-off as required. |
|
Let me do a quick push to address your comment #158 (comment) |
commit ee12044 brought in this dependency on runc's libcontainer/devices package. The runc maintainers consider "libcontainer" to be unstable, and not to be used by external consumers. The package (due to it being part of a large module) also brings in many transitive dependencies. Given that the DeviceFromPath is only a small wrapper for some golang.org/x/sys calls, and the main purpose of the utility (giving information about "cgroup_permissions (which cannot be easily queried)") was not used, we can replace it with a more customised implemenation that's targetted at the information that's needed here. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
no_libcontainer
branch
from
15f6c80 to
acec772
Compare
|
rebased, and addressed 👍 (needs "approval" again to run CI - sorry for that!) |
|
merging bypassing the DCO check. Confirmed that commit was signed-off. |
|
Thanks! Do you know if there were plans for a new tag? (I'll try to keep an eye, but if there's a new tag, I can update the version in containerd 😄) |
I've added the DCO bot at the org level for all repos. Please let me know if that fixes the problem going forward. |
Thanks @thaJeztah. I will check with the other members of the WG to tag and release the update. |
|
Thanks! I opened a draft PR in containerd to verify everything works with the latest (at least: in "CI") |
commit ee12044 brought in this dependency on runc's libcontainer/devices package.
The runc maintainers consider "libcontainer" to be unstable, and not to be used by external consumers. The package (due to it being part of a large module) also brings in many transitive dependencies.
Given that the DeviceFromPath is only a small wrapper for some golang.org/x/sys calls, and the main purpose of the utility (giving information about "cgroup_permissions (which cannot be easily queried)") was not used, we can replace it with a more customised implemenation that's targetted at the information that's needed here.