Skip to content

fix: filegroup circular deps #164

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

birunts
Copy link
Contributor

@birunts birunts commented May 28, 2025

Previously generated filegroup targets can contain dependency cycle.

The case here is that defined filegroup of a package should not have deps of other packages but rather to its :data package.

This change removes potential dependency cycles and makes references to package's :data.

Resolves #163 & Closes #163.

Previously generated `filegroup` targets can contain
dependency cycle.

The case here is that defined filegroup of a package
should not have deps of other packages but rather to
its `:data` package.

This change removes potential dependency cycles and
makes references to package's `:data`.
@thesayyn
Copy link
Collaborator

I don't think this solves the problem. By not depending on the :data target only, you are excluding the transitive dependencies, that's why you don't see the problem anymore but this basically breaks everything if i am not mistaken.

@birunts
Copy link
Contributor Author

birunts commented May 29, 2025

Agree. But currently package filegroups are defined wrong and you cannot even use it (bazelisk build @bookworm//libgcc-s1:libgcc-s1). If package should be done this way to contain transitive dependencies, definitely it cannot be filegroup but rather custom rule who will be able to handle that somehow.

From another side gathering all needed _PACKAGES is pointless as if packages should handle transitive dependencies then this list should contain only those explicitly specified in bookworm.yaml file where transitive deps will be resolved.
So even better that real filegroup package (e.g. @bookworm//libgcc-s1/amd64:amd64) do not contain deps but only its :data and

Indeed current change is not a fix but rather a quick workaround. What I see now, the real package (e.g. @bookworm//libgcc-s1/amd64:amd64) is a custom rule who contains package :data only and returns a provider with its (Depends,Recommends,Suggests,Breaks) dependencies similarly as dep packages do.
Then main package container is also a custom rule who can use that provider(s) and produce a target with full list of packages :data to be installed.

At least this is mine understanding of current state 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cycle error when using all-in-one target
2 participants