-
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
feat: add convience targets #115
Conversation
cc @jjmaestro |
a485741
to
423a3cd
Compare
423a3cd
to
e64dd34
Compare
@thesayyn so, part of this is a big chunk of "the top of the stack" of PRs (#100)... and again, given the amount of changes that this will introduce, I will have to re-do a ton of the intermediate PRs in the stack that have been 2 months waiting to be reviewed. Just FYI, it's very demotivating to see a big chunk of the functionality that made me revamp the repo being implemented without the rest of the changes. And again, this IMHO "slaps it in" while I built the refactor to clean all of the parts. Again, "2 months in the making". |
Just more context of how I would have done something like this... at the very least, you could have eyeballed the stack and considered if you are (mostly) going to merge it in or not. And if so, you would have implemented this on top of #100, adding the extra convenience aliases that you are adding. That would help a contributor like me to feel like the work that's been done is worth something. Instead, I now have to scramble and rebase everything with larger conflicts. |
I didn't see you made a similar improvement. apologies on my end for that. Big PRs are hard to review and i don't have the time to look at them at once. In my defense refactor PRs are mostly "i don't like the way you did things so i am gonna just change it". I am not trying to be extra, if you look at this you will see that i took the time to actually review and land a lot of PRs. And the reason pinged you was to get your opinions as you were trying to make this ruleset better. :) |
It's also fair to mention that i am not getting paid or being thanked either for the most of the work i do here. |
IMO refactoring is definitely not "I don't like how you did this". Most of the time it should be to make the code more maintainable so that other changes become easier. That's how I've tried to make all the changes in the stack. |
I know, neither are any of the contributors! But that's also why I go above and beyond and document the PRs, what they do, each commit has a (hopefully) clear message and concrete change, etc. So that, regardless of how large the PR / stack, it's easy to see the "conducting thread" and have a breadcrumb / trail of everything. And that's why it's disconcerting to see that after 2 months, the reviews (which are good, I'm thankful for that) don't look at the whole stack. Anyway, enough drama. For my part, sorry for dumping this on you. Will go over the changes over this week and will try to get the PRs in the stack rebased. |
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.
Reviewing the PR, and acknowledging I'm very biased because of the outstanding PRs I have done... it definitely looks like this could be implemented easily / easier and IMO nicer on top of #100 😕 It's basically #100 plus some aliases that are quite useful and a very nice approach to generate the Starlark code (VS my crappier ad-hoc approach).
It's probably much to ask but I would appreciate if we could re-do this on top of #100, taking all the cleanups for the lockfile
and deb_import
. I really like the starlark_codegen_utils.bzl
part, I think it would clean up a lot of the code I wrote in deb_import
to do the templates.
I'll probably try to re-implement this on top of #100 to see how it looks like, I would really like to try and have it all on the PR stack to avoid more churn (it's taken me days to re-do the PRs 😞 )
Tthe tests are failing in Here's the patch to fix diff --git a/apt/private/BUILD.bazel b/apt/private/BUILD.bazel
index 19a4af1..cadebf9 100644
--- a/apt/private/BUILD.bazel
+++ b/apt/private/BUILD.bazel
@@ -27,6 +27,8 @@ bzl_library(
visibility = ["//apt:__subpackages__"],
deps = [
":lockfile",
+ ":starlark_codegen_utils",
+ "@bazel_skylib//lib:new_sets",
"@bazel_tools//tools/build_defs/repo:cache.bzl",
"@bazel_tools//tools/build_defs/repo:http.bzl",
"@bazel_tools//tools/build_defs/repo:utils.bzl",
@@ -95,3 +97,9 @@ bzl_library(
visibility = ["//apt:__subpackages__"],
deps = [":version"],
)
+
+bzl_library(
+ name = "starlark_codegen_utils",
+ srcs = ["starlark_codegen_utils.bzl"],
+ visibility = ["//apt:__subpackages__"],
+) |
Also found a bug that breaks in diff --git a/apt/private/deb_translate_lock.bzl b/apt/private/deb_translate_lock.bzl
index 6491868..509062c 100644
--- a/apt/private/deb_translate_lock.bzl
+++ b/apt/private/deb_translate_lock.bzl
@@ -169,11 +169,11 @@ def _deb_translate_lock_impl(rctx):
_PACKAGE_TEMPLATE.format(
target_name = package["name"],
data_targets = starlark_codegen_utils.to_dict_attr({
- "//:linux_%s" % arch: "//%s/%s" % (package["name"], package["arch"])
+ "//:linux_%s" % arch: "//%s/%s" % (package["name"], arch)
for arch in architectures._values
}),
control_targets = starlark_codegen_utils.to_dict_attr({
- "//:linux_%s" % arch: "//%s/%s:control" % (package["name"], package["arch"])
+ "//:linux_%s" % arch: "//%s/%s:control" % (package["name"], arch)
for arch in architectures._values
}),
), |
Aha, thank you. one strong reason to not change things because it feels better :) |
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
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.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
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.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.
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.
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.
* chore: pre-commit run --all-files Ditto * fix: different _PACKAGES, :packages and :dpkg_status per architecture 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 #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. * fix: sync examples/debian_snapshot and e2e/smoke test 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. * fix: debian_snapshot architecture 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. * feat: add more Debian architectures `_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. * fix: starlark_codegen_utils.to_dict_list_attr The list inside the dict should be formatted and indented. * fix: buildifier
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 :-?
Currently targets generated by apt is really crude, forces all packages to be enumerated again in the BUILD file.
This PR improves the API surface by introducing a few new convenience targets.
@bullseye//:bullseye
target at the external repository root which has the same name as the repo that contains all theinstalled packages
+ adpkg_status target
for all installed packages to be found by scanners.@bullseye//bash
target that is alias to architecture specific packages with aselect
to allow same oci_image target to be transitioned to different platforms.@bullseye//:packages
target: to access all package archives (including transitives)@bullseye//:dpkg_status
target: to access a dpkg_status archive (including transitives)