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

mv contrib/cmd tests/cmd (except memfd-bind) #4377

Merged

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Aug 14, 2024

The following commands are moved from contrib/cmd to tests/cmd:

  • fs-idmap
  • pidfd-kill
  • recvtty
  • remap-rootfs
  • sd-helper
  • seccompagent

So as to clarify that distro maintainers should not include these binaries in their package.

ref: https://bugs.launchpad.net/ubuntu/+source/runc/+bug/2076981

rata
rata previously approved these changes Aug 14, 2024
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.

This LGTM, but do you think this will be enough?

Maybe we can change the Makefile in some way, so those don't get build except for tests? Or print in "make all" some warning that test binaries are created, for a release you should use "make XXX"?

@AkihiroSuda AkihiroSuda force-pushed the distro-should-not-install-recvtty-etc branch from bc54f44 to 22583fc Compare August 14, 2024 10:35
@AkihiroSuda AkihiroSuda added the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Aug 14, 2024
@AkihiroSuda
Copy link
Member Author

Updated make all to print a note.

@AkihiroSuda AkihiroSuda force-pushed the distro-should-not-install-recvtty-etc branch from 22583fc to d86d8e9 Compare August 14, 2024 18:09
Makefile Outdated
@@ -79,6 +79,8 @@ runc-bin: runc-dmz

.PHONY: all
all: runc recvtty sd-helper seccompagent fs-idmap memfd-bind pidfd-kill remap-rootfs
@echo "*Note*: Only the runc binary should be installed to user environments."
Copy link
Member

Choose a reason for hiding this comment

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

Please add a new line after *Note*: , or add 8 spaces before Other binaries...?
I think it will looks more beautiful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like

@echo "To distro maintainers: this is not make target you're looking for. Use runc or runc-static."

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe don't make make more talkative at all.

@kolyshkin
Copy link
Contributor

kolyshkin commented Aug 15, 2024

In fact, we have 8 7 entries in there.

  • 7 6 of them are used by tests/integration, and should probably be moved there (say to tests/cmd/). The only other thing to do is to fix the link to recvtty from docs/terminal.md.
  • 1 (memfd-bind) can be kept as-is, its documentation is adequately telling distro maintainers what to do with it.

Edited: I miscounted it last night :)

@rata rata dismissed their stale review August 15, 2024 09:29

New approach suggested seems better

The following commands are moved from `contrib/cmd` to `tests/cmd`:
- fs-idmap
- pidfd-kill
- recvtty
- remap-rootfs
- sd-helper
- seccompagent

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda AkihiroSuda force-pushed the distro-should-not-install-recvtty-etc branch from d86d8e9 to f76489f Compare August 15, 2024 16:00
@AkihiroSuda AkihiroSuda changed the title contrib/cmd: add README.md to clarify the intent mv contrib/cmd tests/cmd (except memfd-bind) Aug 15, 2024
@AkihiroSuda
Copy link
Member Author

Updated as suggested

@AkihiroSuda AkihiroSuda removed area/docs backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 labels Aug 15, 2024
@thaJeztah
Copy link
Member

Would moving things to internal/ be something we should consider? It makes it crystal clear "for own use"?

@kolyshkin
Copy link
Contributor

Would moving things to internal/ be something we should consider? It makes it crystal clear "for own use"?

When writing the above comment yesterday, I was going to suggest internal/cmd but then realized these are mostly used for tests, and we have a directory for that already.

I think tests/cmd is a good enough path to figure out these binaries are for tests and thus should not be included into runc package.

Also, two of the programs (recvtty and seccomagent) are also used as code examples, so they are not entirely "internal".

Comment on lines +16 to +21
RECVTTY="${INTEGRATION_ROOT}/../../tests/cmd/recvtty/recvtty"
SD_HELPER="${INTEGRATION_ROOT}/../../tests/cmd/sd-helper/sd-helper"
SECCOMP_AGENT="${INTEGRATION_ROOT}/../../tests/cmd/seccompagent/seccompagent"
FS_IDMAP="${INTEGRATION_ROOT}/../../tests/cmd/fs-idmap/fs-idmap"
PIDFD_KILL="${INTEGRATION_ROOT}/../../tests/cmd/pidfd-kill/pidfd-kill"
REMAP_ROOTFS="${INTEGRATION_ROOT}/../../tests/cmd/remap-rootfs/remap-rootfs"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: all these should now be like "${INTEGRATION_ROOT}/../cmd/remap-rootfs/remap-rootfs"

@@ -214,7 +214,7 @@ function scmp_act_notify_template() {
@test "runc run [seccomp] (SCMP_ACT_NOTIFY example config)" {
# Run the script used in the seccomp agent example.
# This takes a bare config.json and modifies it to run an example.
"${INTEGRATION_ROOT}/../../contrib/cmd/seccompagent/gen-seccomp-example-cfg.sh"
"${INTEGRATION_ROOT}/../../tests/cmd/seccompagent/gen-seccomp-example-cfg.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"${INTEGRATION_ROOT}/../../tests/cmd/seccompagent/gen-seccomp-example-cfg.sh"
"${INTEGRATION_ROOT}/../cmd/seccompagent/gen-seccomp-example-cfg.sh"

Comment on lines +16 to +21
RECVTTY="${INTEGRATION_ROOT}/../../tests/cmd/recvtty/recvtty"
SD_HELPER="${INTEGRATION_ROOT}/../../tests/cmd/sd-helper/sd-helper"
SECCOMP_AGENT="${INTEGRATION_ROOT}/../../tests/cmd/seccompagent/seccompagent"
FS_IDMAP="${INTEGRATION_ROOT}/../../tests/cmd/fs-idmap/fs-idmap"
PIDFD_KILL="${INTEGRATION_ROOT}/../../tests/cmd/pidfd-kill/pidfd-kill"
REMAP_ROOTFS="${INTEGRATION_ROOT}/../../tests/cmd/remap-rootfs/remap-rootfs"
Copy link
Contributor

Choose a reason for hiding this comment

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

IOW

Suggested change
RECVTTY="${INTEGRATION_ROOT}/../../tests/cmd/recvtty/recvtty"
SD_HELPER="${INTEGRATION_ROOT}/../../tests/cmd/sd-helper/sd-helper"
SECCOMP_AGENT="${INTEGRATION_ROOT}/../../tests/cmd/seccompagent/seccompagent"
FS_IDMAP="${INTEGRATION_ROOT}/../../tests/cmd/fs-idmap/fs-idmap"
PIDFD_KILL="${INTEGRATION_ROOT}/../../tests/cmd/pidfd-kill/pidfd-kill"
REMAP_ROOTFS="${INTEGRATION_ROOT}/../../tests/cmd/remap-rootfs/remap-rootfs"
RECVTTY="${INTEGRATION_ROOT}/../cmd/recvtty/recvtty"
SD_HELPER="${INTEGRATION_ROOT}/../cmd/sd-helper/sd-helper"
SECCOMP_AGENT="${INTEGRATION_ROOT}/../cmd/seccompagent/seccompagent"
FS_IDMAP="${INTEGRATION_ROOT}/../cmd/fs-idmap/fs-idmap"
PIDFD_KILL="${INTEGRATION_ROOT}/../cmd/pidfd-kill/pidfd-kill"
REMAP_ROOTFS="${INTEGRATION_ROOT}/../cmd/remap-rootfs/remap-rootfs"

(not tested)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can be another PR?

Copy link
Contributor

@kolyshkin kolyshkin 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!

@rata rata merged commit 41831e7 into opencontainers:main Aug 21, 2024
40 checks passed
@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