Skip to content

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

Merged
rata merged 1 commit into
opencontainers:mainfrom
AkihiroSuda:distro-should-not-install-recvtty-etc
Aug 21, 2024
Merged

mv contrib/cmd tests/cmd (except memfd-bind)#4377
rata merged 1 commit into
opencontainers:mainfrom
AkihiroSuda:distro-should-not-install-recvtty-etc

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda commented Aug 14, 2024 •
edited
Loading

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
View reviewed changes
Copy link
Copy Markdown
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/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Aug 14, 2024
@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Aug 14, 2024

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
lifubang
lifubang reviewed Aug 15, 2024
View reviewed changes
Comment thread Makefile Outdated

.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
Copy Markdown
Member

@lifubang lifubang Aug 15, 2024

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
Copy Markdown
Contributor

@kolyshkin kolyshkin Aug 15, 2024

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
Copy Markdown
Contributor

@kolyshkin kolyshkin Aug 15, 2024

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
Copy Markdown
Contributor

kolyshkin commented Aug 15, 2024 •
edited
Loading

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

@AkihiroSuda
mv contrib/cmd tests/cmd (except memfd-bind)
f76489f 
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 <akihiro.suda.cz@hco.ntt.co.jp>
@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
Copy Markdown
Member Author

AkihiroSuda commented Aug 15, 2024

Updated as suggested

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

thaJeztah commented Aug 15, 2024

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

@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Aug 15, 2024

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".

kolyshkin
kolyshkin reviewed Aug 16, 2024
View reviewed changes
Comment thread tests/integration/helpers.bash
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
Copy Markdown
Contributor

@kolyshkin kolyshkin Aug 16, 2024

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"

kolyshkin
kolyshkin reviewed Aug 16, 2024
View reviewed changes
Comment thread tests/integration/seccomp-notify.bats
# 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
Copy Markdown
Contributor

@kolyshkin kolyshkin Aug 16, 2024

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"

kolyshkin
kolyshkin reviewed Aug 16, 2024
View reviewed changes
Comment thread tests/integration/helpers.bash
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
Copy Markdown
Contributor

@kolyshkin kolyshkin Aug 16, 2024

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
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda Aug 16, 2024

Choose a reason for hiding this comment

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

Can be another PR?

kolyshkin
kolyshkin approved these changes Aug 19, 2024
View reviewed changes
Copy link
Copy Markdown
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

rata
rata approved these changes Aug 21, 2024
View reviewed changes
Copy link
Copy Markdown
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
@kolyshkin kolyshkin mentioned this pull request Aug 22, 2024
Merged
@lifubang lifubang mentioned this pull request Aug 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lifubang lifubang lifubang left review comments

@rata rata rata approved these changes

@kolyshkin kolyshkin kolyshkin approved these changes

Assignees

No one assigned

Labels

easy-to-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AkihiroSuda @kolyshkin @thaJeztah @rata @lifubang