Fix circular dependencies #116

manuelnaranjo · 2025-02-27T10:30:44Z

Make sure the lock files don't contain circular dependencies, solving the current bugs

github-actions · 2025-02-27T10:32:37Z

⚠️ Optional job e2e-bzlmod-toolchain-circular-dependencies failed ⚠️

exit status: 1

manuelnaranjo · 2025-02-27T10:37:54Z

⚠️ Optional job e2e-bzlmod-toolchain-circular-dependencies failed ⚠️

exit status: 1

Actually no, but fine this job is going away

cmd/config_helper.go

bazeldnf/extensions.bzl

cmd/root.go

kellyma2 · 2025-03-11T18:30:28Z

internal/rpm.bzl

 def _rpm_impl(ctx):
    if ctx.attr.urls:
-        downloaded_file_path = "downloaded"
+        downloaded_file_path = ctx.attr.urls[0].split("/")[-1]


why do we need to change this?

pkg/rpm/tar_test.go

kellyma2 · 2025-03-11T18:37:23Z

cmd/config_helper.go

-	packageNames := keys(allPackages)
-	sortedPackages := make([]*bazeldnf.RPM, 0, len(packageNames))
-	for _, name := range packageNames {
+	slices.Sort(targets)


why do we care about the order of the targets?

To make the lock file generation consistent and stable, we don't want multiple runs of the same required packages against the same db to be changing due to sorting differences

kellyma2 · 2025-03-11T18:39:09Z

cmd/config_helper.go

-		pkg.SetDependencies(deps)
+			pkg.SetDependencies(deps)
+		} else {
+			pkg.SetDependencies(nil)


this is presumably to make it so that packages that we didn't directly request reduce the fanout they drag in?

It looks like we'll probably fail later rounds of collecting dependencies if we have a scenario like:
A -> B -> D
B -> D
C -> B -> D

In this scenario it (superficially) looks like we're going to to drop B's dependency on D before we process C.

It'd be good if we can separate out the traversal part here via something like https://github.com/dominikbraun/graph which will allow us to do a simple topological sort which we can then transform into the appropriate lockfile outputs.

I think now this is fixed

Make sure only those packages that the lock file required contain the full qualified list of dependencies, this allows to avoid circular dependencies in bazel land

Now the lock file is in the expected state, make the repo config a bit stricter to test even more functionality

Now that we have fixed the bug we can make this job actually fail

In lockfile mode we need to make sure that only those RPMs requested at the time of the lockfile can be depend upon, otherwise we allow users to consume from RPMs that may not have their entire dependency tree populated.

Make sure the depsets are working as expected to avoid getting duplicates in the final list of dependencies going into rpmtree

Make sure circular dependencies are resolved when rendering the lock file, we flatten the dependency list of all requested packages.

When consuming the lock file make sure we only make use of the top level dependencies ignoring transitive ones.

Make sure we have a test with a fake lock file that simulates a totally circular dependency tree to validate that starlark behaves as expected

Make sure we let the user know to not use incompatible bazel versions, as there are bugs that lead to issues like [this one]( bazel-contrib/rules_jvm_external#1205 )

Make sure that when + is found in a package name and ignore_deps is set to False, then the visible package name also gets + replaced with plus

kellyma2 · 2025-04-16T07:12:50Z

bazeldnf/extensions.bzl

    lock_file_json = json.decode(content)

+    if not config.ignore_deps:
+        if versions.is_at_least("7", versions.get()) and not versions.is_at_least("7.4.0", versions.get()):


where are these constraints coming from?

I don't remember exactly why, what I do remember is setting the bazel version to 7.0 and so until 7.4.0 with bazelisk and it was failing with some weird error, same for 8.x

Probably worth digging into this a bit more. We're effectively saying that we no longer support pre 7.4 versions of bazel? That's a fairly strong assertion.

kellyma2 · 2025-04-23T21:55:51Z

internal/rpm.bzl

    "integrity": attr.string(),
    "dependencies": attr.label_list(
        mandatory = False,
-        providers = [RpmInfo],


why do we want to drop this?

kellyma2 · 2025-04-23T21:58:27Z

cmd/config_helper_test.go

+				"repository": []string{},
+			},
+			expectedRPMs: []*bazeldnf.RPM{
+				newSimpleRPM("package1", "package2", "package4"),


why wouldn't package2 also have package4 in this case?

kellyma2 · 2025-04-23T22:01:44Z

cmd/config_helper.go

+	slices.Sort(targets)
+	sortedPackages := make([]*bazeldnf.RPM, 0, len(allPackages))
+
+	requested := make(map[string]bool)


should use empty struct instead of bool here as the former uses zero bytes

kellyma2 · 2025-04-23T22:04:13Z

cmd/config_helper.go

+	for _, name := range sortedKeys(allPackages) {
 		pkg := allPackages[name]
-		deps, err := collectDependencies(name, pkg.Dependencies, providers, ignored)
+		if _, requested := requested[name]; !requested {


should use ok here instead of requested because that's the normal pattern and this overloads the variable name

kellyma2 · 2025-04-23T22:07:33Z

cmd/config_helper.go

-	sortedPackages := make([]*bazeldnf.RPM, 0, len(packageNames))
-	for _, name := range packageNames {
+	slices.Sort(targets)
+	sortedPackages := make([]*bazeldnf.RPM, 0, len(allPackages))


maybe move this down to where sortedPackages is constructed so it's a bit more clear what's happening

kellyma2 · 2025-04-23T22:07:55Z

cmd/config_helper.go

+			continue
+		}
+
+		pkg.SetDependencies(nil)


why set dependencies to nil here?

kellyma2 · 2025-04-23T22:09:38Z

cmd/config_helper.go

+	}
+
+	slices.SortFunc(sortedPackages, func(a, b *bazeldnf.RPM) int {
+		return cmp.Compare(a.Name, b.Name)


isn't the sorting already implicit by virtue of having constructed sortedPackages with the sorted keys?

kellyma2 · 2025-04-23T22:10:48Z

cmd/config_helper.go

 }

-func collectDependencies(pkg string, requires []string, providers map[string]string, ignored map[string]bool) ([]string, error) {
+func collectDependencies(pkg string, requires []string, providers map[string]string, ignored map[string]bool, allPackages map[string]*bazeldnf.RPM) ([]string, error) {


can we use something like https://pkg.go.dev/github.com/dominikbraun/graph to do a topological sort of the dependencies here and just get the result of that?

kellyma2 · 2025-04-23T22:11:42Z

cmd/config_helper.go

 	depSet := make(map[string]bool)
-	for _, req := range requires {
+	explored := make(map[string]bool)
+	for len(requires) > 0 {


Can we just use something like https://pkg.go.dev/github.com/dominikbraun/graph and do a topological sort?

manuelnaranjo marked this pull request as ready for review February 27, 2025 10:39

manuelnaranjo requested a review from kellyma2 February 27, 2025 10:39

kellyma2 reviewed Feb 27, 2025

View reviewed changes

cmd/config_helper.go Outdated Show resolved Hide resolved

cmd/config_helper.go Outdated Show resolved Hide resolved

manuelnaranjo force-pushed the mnaranjo/fix-circular-dependencies branch 3 times, most recently from b89d6cf to 2b54fb0 Compare February 28, 2025 08:48

manuelnaranjo self-assigned this Mar 4, 2025

manuelnaranjo force-pushed the mnaranjo/fix-circular-dependencies branch 4 times, most recently from 0f6989d to 8e05911 Compare March 7, 2025 12:19

kellyma2 reviewed Mar 11, 2025

View reviewed changes

manuelnaranjo force-pushed the mnaranjo/fix-circular-dependencies branch from 8e05911 to 42d52c4 Compare March 19, 2025 08:55

manuelnaranjo added 10 commits April 9, 2025 19:51

resolution: drop circular dependencies

12270fc

Make sure only those packages that the lock file required contain the full qualified list of dependencies, this allows to avoid circular dependencies in bazel land

test: Improving circular dependencies test

2785697

Now the lock file is in the expected state, make the repo config a bit stricter to test even more functionality

ci: marking the job as fixed

b34a52e

Now that we have fixed the bug we can make this job actually fail

bazel: make sure only top level RPMs are public

6671594

In lockfile mode we need to make sure that only those RPMs requested at the time of the lockfile can be depend upon, otherwise we allow users to consume from RPMs that may not have their entire dependency tree populated.

ci: test extension for duplicates

f354844

Make sure the depsets are working as expected to avoid getting duplicates in the final list of dependencies going into rpmtree

go: fix circular dependencies

06cc856

Make sure circular dependencies are resolved when rendering the lock file, we flatten the dependency list of all requested packages.

bazel: use top level dependencies only

cbdca02

When consuming the lock file make sure we only make use of the top level dependencies ignoring transitive ones.

ci: adding a test for recursive dependencies

e40f809

Make sure we have a test with a fake lock file that simulates a totally circular dependency tree to validate that starlark behaves as expected

bazel: we need 7.4+ or 8.1+

9297d0b

Make sure we let the user know to not use incompatible bazel versions, as there are bugs that lead to issues like [this one]( bazel-contrib/rules_jvm_external#1205 )

bug: fix rename bug

4e0b823

Make sure that when + is found in a package name and ignore_deps is set to False, then the visible package name also gets + replaced with plus

manuelnaranjo force-pushed the mnaranjo/fix-circular-dependencies branch from 42d52c4 to 4e0b823 Compare April 9, 2025 17:53

kellyma2 reviewed Apr 16, 2025

View reviewed changes

kellyma2 reviewed Apr 23, 2025

View reviewed changes

Fix circular dependencies #116

Are you sure you want to change the base?

Fix circular dependencies #116

Uh oh!

Conversation

manuelnaranjo commented Feb 27, 2025

Uh oh!

github-actions bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Optional job e2e-bzlmod-toolchain-circular-dependencies failed ⚠️

Uh oh!

manuelnaranjo commented Feb 27, 2025

⚠️ Optional job e2e-bzlmod-toolchain-circular-dependencies failed ⚠️

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Feb 27, 2025 •

edited

Loading