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

docker-moby: enable buildx plugin #30

Open
wants to merge 1 commit into
base: nilrt/master/next
Choose a base branch
from

Conversation

usercw88
Copy link
Contributor

@usercw88 usercw88 commented Nov 21, 2024

AB#2939060

Note: This is not intended to be submitted to ni/meta-virtualization
The goal is to submit this to meta-virtualization (scarthgap) upstream

Testing:
[x] Built docker-moby locally and verified the creation of docker-buildx binary, docker-buildx-plugin package
[x] Installed docker-buildx-plugin onto a system and verified creating "BuildKit" containers function correctly

Copy link

@chaitu236 chaitu236 left a comment

Choose a reason for hiding this comment

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

This is not intended to be submitted to ni/meta-virtualization
The goal is to submit this to meta-virtualization (scarthgap) upstream

Seeing that buildx was an optional plugin, are we sure upstream will accept this change building the plugin by default? If docker can work without buildx, I suspect this change may not be accepted upstream.

@@ -166,3 +174,7 @@ FILES:${PN}-cli += "${bindir}/docker"
# NOT rdepend to get a classic one-package install
DOCKER_UNIFIED_PACKAGE ?= "True"
RDEPENDS:${PN} += "${@bb.utils.contains("DOCKER_UNIFIED_PACKAGE", "True", "${PN}-cli", "", d)}"

PACKAGES =+ "${PACKAGE_NAME}-buildx-plugin"

Choose a reason for hiding this comment

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

Why ${PACKAGE_NAME} and not ${PN}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

${PN} would result in "docker-moby-buildx-plugin"
${PACKAGE_NAME} would result in "docker-buildx-plugin"

According to https://docs.docker.com/engine/install/centos/#installation-methods (referenced in the above comment) the buildx-plugin is not intended to have "moby" or "ce" within it's package name

@usercw88
Copy link
Contributor Author

Seeing that buildx was an optional plugin, are we sure upstream will accept this change building the plugin by default? If docker can work without buildx, I suspect this change may not be accepted upstream.

Not sure if they will allow it, but it's worth a try?

@chaitu236
Copy link

Not sure if they will allow it, but it's worth a try?

It's up to you but I'd ask why this should be part of docker.inc and not a separate docker-buildx.bb recipe so that users that don't care about buildx don't get increased build times due to adding buildx to docker.inc.

@chaitu236
Copy link

FYI I'd be ok with just ni/meta-nilrt#767 and call it a day if disabling BUILDX doesn't affect our container tests.

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.

2 participants