refactor: package_index

[jjmaestro]

Note

Stacked on top of #92

refactor: apt_dep_resolver_test

Cleanup apt_dep_resolver_test removing the apt_deb_repository mock, it's not really needed once we have proper testing and mocking of apt_deb_repository that we can reuse.

test: apt_deb_repository

• Add testing for apt_deb_repository mocking the external / side effects (downloads, decompression, etc).
• Rename _parse_repository back to _parse_package_index

refactor: cleanup _resolve_package virtual_package logic

• Refactor the virtual_package loop moving if version to the outside

feat: add parse_provides

• Add a parse_provides method which parses and validates 'Provides'

refactor: nested_dict struct

• Add a helper nested_dict struct that encapsulates the get-set logic
• Remove all the (now) unnecessary wrappers like _package_versions, _virtual_packages, etc.
• Fix get() method to return default_value if no keys are given
• Add add() to nested_dict struct so that we can simplify the virtual package logic in apt_deb_repository.bzl _add_package

refactor: _resolve_all

• Add assertions for unresolved_deps in apt_dep_resolver_test
• Rename resolve_all to resolve
• Reorder (name, version, arch) args to match the order of the index keys (arch, name, version)
• Break up the long lines in comments
• Improve _ITERATION_MAX_ fail message

refactor: _parse_version_constraint

Cleanup the version constraint parsing using the VERSION_OPERATORS that we now have in the version struct plus also validating the version being parsed.

chore: mv resolution_test to apt_dep_resolver_test

Tests should match the file they are testing

chore: move version_constraint tests to their own test file

version_constraint.bzl is a bunch of utils for package version dependency. Either it has enough entity to stand by itself or the functionality should be folded into version.bzl and package_index.bzl and/or package_resolution.bzl.

refactor: _version_relop

Refactor _version_relop into a compare method in version.bzl plus a VERSION_OPERATORS dict so that (1) we use the operator strings everywhere and (2) we can use the keys to validate the operators.

refactor: _fetch_package_index

While working with some flaky mirrors and trying to figure out why they were failing I found the _fetch_package_index code a bit hard to follow so here's my attempt at streamlining it a bit:

• Change the general flow of the for-loop so that we can directly set the reasons for failure in failed_attempts
• Remove both integrity as an argument and as a return value since neither is ever used.
• return the content of the Packages index instead of the path so that (1) we don't need rctx anywhere else and (2) is easier to mock.
• Reword failure messages adding more context and debug information
• Shorter lines and templated strings, trying to make the code easier to read and follow.

refactor: apt_dep_resolver

• Remove unnecessary state struct, just pass the repository struct to the resolver methods
• Remove unused resolve_package from the public API

[jjmaestro]

@thesayyn I've re-done the PR on top of all of the new changes. I've now left the separation of apt_deb_repository.bzl and apt_dep_resolver.bzl.

I'll wait before rebasing the other PRs just in case you want changes in this one. I've re-done the whole stack of PRs because the changes were so big they needed to be fully re-done (manual patching files, git rebase was too hard / "not possible").

Please enable the checks workflow and review it when you can.

Thanks!

[thesayyn]

i don't think there is value in changing things just because it feels better. I can't help but say that most of these changes unnecessary in my opinion. I really don't see how they better objectively. There are some fixes here worth, i'd be happy to land those if you feel like splitting them.

It makes it equally hard for me to review this and decide if something is needed.

[jjmaestro]

@thesayyn the changes were done to accommodate the stack of diffs on top, where I've refactored the lockfile (#95), the manifest (#94) the templates to deb_import (#96) and finally the architecture targets (#100) that you just landed in #115.

I'm sorry that you feel many of these are just "because it feels better". It would be good if you could point at the things you feel are that way.

[jjmaestro]

There are some fixes here worth, i'd be happy to land those if you feel like splitting them.

This is probably the easiest, let me know what you want from the stack and I'll start splitting them into PRs.

[thesayyn]

We can revert this, this is more involved than i would like the util .set/get_dict to be.

[jjmaestro]

I thought it made sense to have it as a first-class data struct rather than just a helper function on a dict. The struct ensures the access to the dict is through the get-set while just having the functions doesn't prevent any direct change to the dict.

Also, I was using the functionality in other places in the code (e.g. the lockfile refactor) so having it all encapsulated in one data struct made more sense to me.

Do you still prefer it back to naked functions over a dict?

[thesayyn]

same as above, revert

[thesayyn]

there is no reason for this to be a map, as its not going to grow overtime or change. I'd rather see this reverted as there is no value here.

[jjmaestro]

As I mentioned in the commit, the value was in being able to validate the strings against the string keys. But I can remove it back to what it was.

[jjmaestro]

This is the relevant commit I was referring to: 4f504f4

diff --git a/apt/private/version_constraint.bzl b/apt/private/version_constraint.bzl
index 93e13e5..fe0e6b4 100644
--- a/apt/private/version_constraint.bzl
+++ b/apt/private/version_constraint.bzl
@@ -2,11 +2,22 @@
 
 load(":version.bzl", version_lib = "version")
 
-def _parse_version_constraint(rawv):
-    vconst_i = rawv.find(" ")
-    if vconst_i == -1:
-        fail('invalid version string %s expected a version constraint ">=", "=", ">=", "<<", ">>"' % rawv)
-    return (rawv[:vconst_i], rawv[vconst_i + 1:])
+def _parse_version_constraint(version_and_constraint):
+    chunks = version_and_constraint.split(" ")
+
+    if len(chunks) != 2:
+        fail("Invalid version constraint %s" % version_and_constraint)
+
+    version_constraint = chunks[0]
+    if version_constraint not in version_lib.VERSION_OPERATORS:
+        msg = "Invalid version constraint: %s\nValid constraints are: %s"
+        fail(msg % (version_constraint, version_lib.VERSION_OPERATORS))
+
+    version = chunks[1]
+
+    version_lib.parse(version)  # parsing version to validate it
+
+    return version_constraint, version
 
 def _parse_dep(raw):
     raw = raw.strip()  # remove leading & trailing whitespace

Previously, the validation was just checking for a space and if not found, it was assuming that the version didn't have a constraint.

With my changes, it (1) checks for the space and fails if none (2) it validates the operator and (3) it validates the version itself.

Is this enough reason for it to be a map? :-?

[thesayyn]

Again, i don't see why this needs to be change, it's already working and there is no value in changing things without a reason. If this change is needed in a larger context, eg if your stack has a fix for something that needs this change, please put it right next to that fix to help me reason why this needs to change.

[jjmaestro]

This was in the same spirit as the test helpers that I added, standardizing on having a _TEST_SUITE_PREFIX constant, etc.

Basically, it was meant to be a clean way to link the stuff that's not publicly accessible for testing purposes only. This way, we always import the module in the test and test the public parts of the struct and if there are parts that are private and we want to test, I thought it would be easier to have a __test__ sub-struct in-place and not have to create another struct, with a long name in there (the __test__ already does signal that it's for testing only, I think).

I'll go back to the DO_NOT_DEPEND_ON_THIS_TEST_ONLY...

[jjmaestro]

@thesayyn I'm really looking into this, I was going to make the change but here's the thing. As mentioned, once I've added proper mocking, the tests don't need any of the _create_test_only() functionality. That is, all of this functionality is superseded by the mocks, which is the "added value".

So, TL;DR added value == better testing, like with the helper stuff you like in the tests. Is that OK? 🙏

[jjmaestro]

I've rebased the branch on top of main, and per your comments, rolled back my changes to _is_satisfied_by and the "leaking" of unmet dependencies.

I've added more context in your other reviews and left them unresolved to hear your opinion, here's a summary re.

• nested_dict: TL;DR I also use it in the next PR in the stack so IMO it's better as its own struct VS helper methods that would leave the nested dict "naked" and available to changes via "non-API" (the get-set + other methods offered in the new nested_dict struct).

• version op map: I use the keys to validate the operator in the commit I linked. If we go back to a struct of functions I think it would require to add a "validate op" method and other code that IMO is not needed by just using "the best data struct" to organize the operator lookup.

• __test__ once I added the mocks and helpers that you mentioned you like, the DO_NOT_DEPEND_ON_THIS_TEST_ONLY struct becomes dead code.

Again, sorry for such a big PR of mix-and-match stuff, it's definitely hard to review it all, regardless of how much context I added in the commits. Thanks a ton for your reviews, happy to discuss this further and continue making the changes you want.

[jjmaestro]

Ran a rebase executing tests and found an issue that I've corrected in the latest push.

[jjmaestro]

Just rebased to remove 2eda7c0 (superseded by #129, now merged in main)