-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix: #120 (and #115?) in MacOS #136
base: main
Are you sure you want to change the base?
Conversation
@thesayyn here's the fix! BTW do you think it would be worth it to let the checks run without needing a review? I think it would make it easier to see if everything is green much faster, without having to bother you with it :-? |
Hmm, i don't think is what we want, either a transition or disabling those tests on macos. |
@thesayyn but I thought the tests that fail were already guarded, as I mentioned in some checks I ran here. What am I missing? :-? If I'm understanding correctly what's going on, the tests are already guarded by the I'm too much of a Bazel n00b, if you can explain it further, I'm happy to check and change "the fix". I agree it definitely looks bad, because it's adding a default state that shouldn't be there, that code should never be executed in the first place, agreed. |
Sorry can’t do that, it’s against the security rules set in place, plus I don’t have permissions to do that. |
Weird, I’ll check later |
Yeah, I've checked again, if I remove % git diff examples/ubuntu_snapshot/BUILD.bazel
diff --git a/examples/ubuntu_snapshot/BUILD.bazel b/examples/ubuntu_snapshot/BUILD.bazel
index 2cb6a91..9856501 100644
--- a/examples/ubuntu_snapshot/BUILD.bazel
+++ b/examples/ubuntu_snapshot/BUILD.bazel
@@ -75,10 +75,4 @@ container_structure_test(
"@platforms//cpu:x86_64": ["test_linux_amd64.yaml"],
}),
image = ":noble",
- target_compatible_with = select({
- "@platforms//cpu:x86_64": ["@platforms//cpu:x86_64"],
- "@platforms//cpu:arm64": ["@platforms//cpu:arm64"],
- }) + [
- "@platforms//os:linux",
- ],
) And I run it manually with
|
Cool, thanks!! |
Maybe what's happening is that
That is, the test target is actually not even running because of the |
Yup, that's it!!! If I add a |
Ah! I see you disabled |
Bazel was throwing an error when running the tests in MacOS even when the Linux tests were guarded with a target_compatible_with that was restricted to Linux. As far as I could tell, this was because running the bazel test with //... was recursively evaluating other targets like //examples/ubuntu_snapshot:_noble_index_json from oci_image and oci_load. When I guard those rules with a target_compatible_with everything works. See GoogleContainerTools#136 for more context and information. Also, seeing that e5f7dc0 also manually commented a test to fix CI for macos so I guess GoogleContainerTools#120 didn't break this, it was probably happening since GoogleContainerTools#115 added the new convenience targets. Finally, maybe this is something that should be fixed in rules_oci, the "inner targets" that it creates should probably be restricted to only run in the `os` and `architecture` given for the image :-?
0d2a8ef
to
068cd02
Compare
Yup! This seems to fix it :)
|
fix: #120 (and #115?) in MacOS
Bazel was throwing an error when running the tests in MacOS even when the Linux tests were guarded with a target_compatible_with that was restricted to Linux.
As far as I could tell, this was because running
bazel test
with//...
was recursively evaluating other targets like//examples/ubuntu_snapshot:_noble_index_json
fromoci_image
andoci_load
. When I guard those rules with a target_compatible_with everything works.Also, seeing that e5f7dc0 also manually commented a test to fix CI for macos so I guess #120 didn't break this, it was probably happening since #115 added the new convenience targets.
Finally, maybe this is something that should be fixed in
rules_oci
, the "inner targets" that it creates should probably be restricted to only run in theos
andarchitecture
given for the image :-?