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

Use platform transition in the oci_image_index rule #531

Closed

Conversation

moroten
Copy link

@moroten moroten commented Mar 19, 2024

The current way of listing images makes Bazel build images for a platform that is not the configured target platform, defined by --platforms. This commit changes the behaviour and applies a transition so that the images are built for the configured target platform.

New way:

oci_image_index(
    name = "app",
    image = ":app_linux",
    platforms = [
        "@io_bazel_rules_go//go/toolchain:linux_amd64",
        "@io_bazel_rules_go//go/toolchain:linux_arm64",
    ]
)

Old way:

oci_image_index(
    name = "app",
    image = [
        ":app_linux_amd64",
        ":app_linux_arm64"
    ],
)

For backwards compatibility, the old way is still possible to use.

The current way of listing images makes Bazel build images for a
platform that is not the configured target platform, defined by
--platforms. This commit changes the behaviour and applies a transition
so that the images are built for the configured target platform.

New way:
oci_image_index(
    name = "app",
    image = ":app_linux",
    platforms = [
        "@io_bazel_rules_go//go/toolchain:linux_amd64",
        "@io_bazel_rules_go//go/toolchain:linux_arm64",
    ]
)

Old way:
oci_image_index(
    name = "app",
    image = [
        ":app_linux_amd64",
        ":app_linux_arm64"
    ],
)

For backwards compatibility, the old way is still possible to use.
@alexeagle
Copy link
Collaborator

Our original intent here was to minimize the code in rules_oci that needs to understand what the user is doing, we liked that transitions occur explicitly. OTOH I do see how this syntax sugar makes it nicer.

Bazel build images for a platform that is not the configured target platform

I'm not sure I follow this existing bug you suggest - don't we build for the same platforms before and after this change? The same targets get transitioned in the same way?

@moroten
Copy link
Author

moroten commented Mar 20, 2024

Bazel build images for a platform that is not the configured target platform

I'm not sure I follow this existing bug you suggest - don't we build for the same platforms before and after this change? The same targets get transitioned in the same way?

Consider the following example:

cc_binary(
    name = "binary_linux_arm64",
)
pkg_tar(
    name = "app_tar_linux_arm64",
    srcs = [":binary_linux_arm64"],
)
oci_image(
    name = "app_image_linux_arm64",
    tars = [":app_tar_linux_arm64"],
)
oci_image_index(
    name = "app_index",
    images = [
        ":app_image_linux_arm64"
    ],
)

I would normally only have a target called binary_linux. Building binary_linux_arm64 with --platforms=//:linux_x86_64 is possible, but makes no sense as the result will be a binary binary_linux_arm64 will not be compatible with arm64. Adding target_compatible_with=[...arm64] will make all _arm64 targets unbuildable on x86_64, which is correct, but :app will also become unbuildable.

The idea here is that all the _linux_arm64 suffixes are removed and that bazel build :app_image --platforms=//:linux_x86_64 and bazel build :app_image --platforms=//:linux_arm64 produce images for respective OS, even if my host is Windows.

cc_binary(
    name = "binary",
)
pkg_tar(
    name = "app_tar",
    srcs = [":binary"],
)
oci_image(
    name = "app_image",
    tars = [":app_tar"],
)

The app_index target itself is actually compatible with any target platform, or at least multiple platforms, i.e. I expect to get the same output independent on --platforms (e.g. building on Windows or MacOS).

@thesayyn
Copy link
Collaborator

#228 (comment)

@moroten
Copy link
Author

moroten commented Mar 20, 2024

Let's keep the discussion about the plan forward in #228.

Are the failing tests due to this change? My feeling is that it is something unrelated. Anyway, some tests should be added for this new functionality.

alexeagle added a commit to alexeagle/bb-storage that referenced this pull request Mar 23, 2024
alexeagle added a commit to alexeagle/bb-storage that referenced this pull request Mar 23, 2024
@thesayyn
Copy link
Collaborator

Can we close this since we don't have to land a change for this?

@thesayyn thesayyn added the can close? We'll close this issue if we don't get a new comment in 30 days. label Jul 25, 2024
@thesayyn thesayyn marked this pull request as draft July 25, 2024 20:35
@thesayyn
Copy link
Collaborator

thesayyn commented Aug 7, 2024

Okay, i am going to close this for house keeping purposes. Thank you for putting time into this.

@thesayyn thesayyn closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can close? We'll close this issue if we don't get a new comment in 30 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants