-
Notifications
You must be signed in to change notification settings - Fork 39
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: platforms, architectures and dpkg_status after #115 #120
Conversation
I am little at unease about the symlinks, this will most likely break BCR, because of rules_distroless/.gitattributes Line 10 in 7b342e2
Can we just improve e2e so it catches this case? |
Ah, I didn't know about BCR! Sure, I'll copy over the test and will make a note in it to keep it in sync. |
ccbc65d
to
e867017
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Thanks for the fixes!
There are small changes to be made but overall good. (sorry for the conflicts.)
Yup, this is a great catch and paves the way for per architecture debian repositories and packages #56 |
b0cc2da
to
7711ba0
Compare
Oh wow, I just fixed the PR and I was checking "Compare" and the rebase still shows!!? even when it's rebased over @thesayyn this, again, will make it difficult to review, sorry 😞 It'll be easier if you check the final state of the commits, but basically, I've done as you requested in your comments. |
OK, I've produced such a diff myself with: $ git co -b fix-pr-115-OLD-rebased b0cc2da
$ git rebase main
$ git rev-parse --short fix-pr-115-OLD-rebased
0f4a221
$ git rev-parse --short fix-pr-115
7711ba0
git diff fix-pr-115-OLD-rebased fix-pr-115 -- . ':(exclude)*.lock.json' But GH doesn't let me attach it as a file to the comment!?? srsly... it seems to only allow PDF, images, movies, etc. Anyway, here you go, in case it makes the review easier: EDIT: I've pushed the old rebased branch so that it's easier to compare via GH with a manual compare link: Compare 0f4a221...7711ba0 |
You have some buildifier errors. |
7711ba0
to
9b2abd4
Compare
There are cases where the packages differ between architectures. E.g. currently in the `examples/ubuntu_snapshot`, `coreutils` depends on `libssl3` on `amd64` but there's no such dependency on `arm64`. Thus, the list of packages and `dpkg_status` has to be different. This failure wasn't caught because this corner case was only happening in the Ubuntu example which was still using "the old `dpkg_status`" that as done by hand, just passing a shorter list of packages and not the full list of installed packages (as implemented in GoogleContainerTools#115). If the test is migrated, when running without the fix it fails with: ERROR: no such package '@@_main~apt~noble//libssl3/arm64': BUILD file not found in directory 'libssl3/arm64' of external repository @@_main~apt~noble. Add a BUILD file to a directory to mark it as a package. ERROR: /home/nonroot/.cache/bazel/_bazel_nonroot/a08c2e4811c846650b733c6fc815a920/external/_main~apt~noble/libssl3/BUILD.bazel:1:6: no such package '@@_main~apt~noble//libssl3/arm64': BUILD file not found in directory 'libssl3/arm64' of external repository @@_main~apt~noble. Add a BUILD file to a directory to mark it as a package. and referenced by '@@_main~apt~noble//libssl3:libssl3' ERROR: Analysis of target '//examples/ubuntu_snapshot:_noble_index_json' failed; build aborted: Analysis failed I've now moved `examples/ubuntu_snapshot` to use the "new" `dpkg_status`. I've also added an explicit test to check that libssl3 is installed in amd64 and another test to check that it's NOT installed in arm64.
These two have been the same but there's been changes to both that weren't replicated / synced into the other. I've now synced and made a comment to remark that it's important to keep these in-sync because testing in e2e with at least the same base test helps in catching bugs like the previous commit, where platforms repo should have been marked as non-dev. Finally, I've moved both tests to Cloudflare's Debian snapshot. Looks like the change happened in f994712 without any explanation and, as usual, I keep finding Debian snapshot extremely flaky and unreliable. Cloudflare has had some issues in the past (e.g. lagging behind replication for a long time) but these are quite rare and resolve much quicker than the Debian snapshot.
Regardless of the `platform_transition_filegroup`, the architecture needs a `select` to properly set the architecture in the manifest. I verified this by running the test and inspecting the generated config JSON. Ideally, this should be encoded in a test but I'm not sure how to go about it.
`_ARCHITECTURE_MAP[arch] or arch` fails when a Debian arch is not in the map. Thus, add all of the Debian architectures that map to platforms CPU architectures. Add "all" arch as well (plus an additional resolution test). Also: * change Debian's ppc64el mapping to the exact matching platform CPU (ppc64le) * move architecture doc links next to _ARCHITECTURE_MAP and add a bunch more links to the Debian wikis.
The list inside the dict should be formatted and indented.
9b2abd4
to
ab4ddd2
Compare
OK, this is with the rebase, now fixing the broken buildifiers left here... |
It's just Readme files over there, which is not related to your changes, you have some unused loads. |
OK, it should be green now! If you also push #135 for |
This made macos unhappy. |
The pre-commit run --all-files that I did and rebased into GoogleContainerTools#120 broke the README :( This fixes it, undoing some of the changes and manually fixing others so that prettier now runs without breaking this Markdown.
@thesayyn :S will look into it now! |
@thesayyn OK, so it's a bit weird, at least to me. I've managed to run all tests in Even so, if I try to run them, I get, as expected
However! If I run
So, somehow, something is running on a previous phase / stage? :-? Anyway, I'll add a default step to the |
There are some tests that also run in macos and Bazel was throwing an error because the `select`s in the package repos needed a default condition.
OK, got it fixed in #136! |
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 :-?
After landing #115 we need
platforms
to be non-dev. This would have been caught in the e2e test but since #115 didn't make changes to the e2e test, it went unnoticed.To prevent this, I've also copied the
examples/debian_snapshot
to thee2e
test. IMHO this helps catch bugs that affect third-party repos usingrules_distroless
as a dependency, such as theplatforms
dependency.I've also verified that we do need the
select
in thearchitecture
, as mentioned in my code review. Otherwise, theConfig
in the manifest is always set toarm64
regardless of the architecture that we are building.I've also fixed the unmapped Debian architectures (e.g. "all"). Basically,
_ARCHITECTURE_MAP[arch] or arch
fails when a Debian arch is not in the map. It should be_ARCHITECTURE_MAP.get(arch, arch)
to default toarch
when it's not in the map. To exercise this and ensure it always works I've:armhf
mapping toarmv7e-mf
ppc64el
mapping to the exact matching platforms CPU (ppc64le
)all
(which is both an arch that's not in the map while being "a special Debian arch" and also a valid platforms CPU constraint)._ARCHITECTURE_MAP
and added a bunch more links to the Debian wikis.Finally, I've fixed
_PACKAGES
,:packages
and:dpkg_status
to be different per-architecture, because there are cases where the packages differ between architectures. E.g. currently in theexamples/ubuntu_snapshot
,coreutils
depends onlibssl3
onamd64
but there's no such dependency onarm64
. Thus, the list of packages anddpkg_status
has to be different.