Ignore circular dependencies in starlark#184
Draft
manuelnaranjo wants to merge 16 commits into
Draft
Conversation
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
3 times, most recently
from
c8199d8 to
e5eaa94
Compare
|
Collaborator
Author
This should no longer be the case once we merge this PR |
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
4 times, most recently
from
cf97632 to
776c3d9
Compare
kellyma2
reviewed
Feb 7, 2026
| if name in registered_rpms: | ||
| return registered_rpms[name] | ||
|
|
||
| registered_rpms[name] = 1 |
Collaborator
There was a problem hiding this comment.
this appears to be redundant
Collaborator
Author
There was a problem hiding this comment.
Mhh it may be the case. Let's remove it and see if it fails
|
|
||
| # Find the RPM in the lookup dict | ||
| rpm = rpm_lookup.get(current) | ||
| if not rpm: |
Collaborator
There was a problem hiding this comment.
why do we want to skip here?
Collaborator
Author
There was a problem hiding this comment.
So while going through the dependency tree we have an entry in the dependency list that's not in the available rpm, this is just defending from a badly crafted lock file. We have some options here, remove the check and let Bazel fail or in the future change the lock file generation process to include the incomplete tree due to excludes and make it obvious where the non complete trees are.
What do you think?
Collaborator
There was a problem hiding this comment.
I think we definitely want to make what's happening here obvious. Maybe we want to do something like (1) cause this to fail and perhaps (2) expose a knob to the user that let's them accept this without failure?
Collaborator
Author
There was a problem hiding this comment.
I added a warning, maybe a strict mode is better and in that case we just fail?
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
from
776c3d9 to
918367a
Compare
Collaborator
Author
|
Will mark as draft until we merge the improved e2e setup |
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
from
918367a to
ed25f2f
Compare
Make sure some golang dependencies are up to date
Start using rules_bazel_integration_test to drive unit tests, this initial step allows us to simplify our e2e testing, next commit will let us speed up the build by avoiding rebuilding the toolchain over and over again
Make sure that all our e2e tests use a prebuilt golang binary for the host platform and that we don't build it once for each test
Make sure all of our e2e use a shared prebuilt bazeldnf binary instead of each one building their own, this simplifies our MODULE.bazel for all of our e2e tests considerably as we no longer to keep prebuilt protoc or golang dependencies
Make sure changes to MODULE.bazel.lock and lock files don't invalidate the cache
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
from
85afee7 to
9bffc4e
Compare
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
from
9bffc4e to
f95a443
Compare
Drive tests entirely through bazel even on CI. Given we're using setup-bazel in our CI we need a little of a workaround for a bug
give each job in the build and test workflow a meaningful name
With the help of Claude I was able to come with a decent contributing doc, of course it needed some cosmetics and modifications, but the result is decent
Make sure we use prebuilt bazeldnf when testing bzlmod releases
Now the PR should include a test report that contains the information from all the tests we execute
RPM dependency tree is insane, it has cycles in it, for instance for bash depends on glibc which depends on glibc-common which depends on bash ( °-° ). Bazel wants DAGs as any decent build system, so the cycles need to be fixed. On the other hand consumers of bazeldnf don't care about glibc they want to add bash to their setups (or whatever they wanted to add which may or may not depend on something else and have a circular dependency), so we can flatten the dependencies and avoid the loops for them. To avoid the cycles we hack things a bit, we first register 1 repository per available rpm in the lock file without dependencies, we call this a blob repository. Then for each requested target (which most likely will become an rpmtree) we link that target to a list of blob repositories. When building the depset of the RpmInfo we will have cases where file will be defined in the case of blob entries or not be defined in the case of publicly facing RPM entries. Depsets are strict in terms of type if the first entry is a file then everything is a file, same if it's None so we need to hack things a little bit. Only the alias entry in the public repository will depend on the rpm target which will contain the dependency tree.
Now we should be able to pass the circular dependencies test
In the case there are duplicated entries in the alias mapping it doesn't necessarily mean a failure mode, it may be due to multiple lock files pointing to the same RPM given this RPM is unique this is not a problem, if it would then bazel would fail to register the repositories
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
from
f95a443 to
9dc668d
Compare
If the same lock file is registered multiple times we reach a bug where blobs needs to be deduplicated
If the extension requests to make an rpmtree for a not known target in the lock file we need to fail with a meaninful error message and not just with a key not known error
Blobs are unique, there's a unique blob repository per rpm url we need to deduplicate blobs are having multiple lock files may lead to overlaping blobs otherwise
manuelnaranjo
force-pushed
the
mnaranjo/ignore-circular-dependencies-in-starlark
branch
from
9dc668d to
4278798
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RPM dependency tree is insane, it has cycles in it, for instance for
bash depends on glibc which depends on glibc-common which depends on
bash ( °-° ). Bazel wants DAGs as any decent build system, so the cycles
need to be fixed.
On the other hand consumers of bazeldnf don't care about glibc they want
to add bash to their setups (or whatever they wanted to add which may or
may not depend on something else and have a circular dependency), so we
can flatten the dependencies and avoid the loops for them.
To avoid the cycles we hack things a bit, we first register 1 repository
per available rpm in the lock file without dependencies, we call this a
blob repository. Then for each requested target (which most likely will
become an rpmtree) we link that target to a list of blob repositories.
When building the depset of the RpmInfo we will have cases where file
will be defined in the case of blob entries or not be defined in the
case of publicly facing RPM entries. Depsets are strict in terms of type
if the first entry is a file then everything is a file, same if it's
None so we need to hack things a little bit.
Only the alias entry in the public repository will depend on the rpm
target which will contain the dependency tree.