Merged
Conversation
alexeagle
requested review from
andyscott,
brandjon,
lberki and
thundergolfer
as code owners
thundergolfer
approved these changes
Oct 25, 2020
thundergolfer
left a comment
thundergolfer
left a comment
There was a problem hiding this comment.
Agree with your argument. Without the ability to express version compatibility across external repositories, any additional dependency added under py_repositories() adds likelihood that end-users experience undocumented breakages.
Looking at #218, the PR that intro'd it over a year ago, there didn't seem to be a great need for it originally, and over time transitive external repo deps have seem to me more fraught with risks. cc @brandjon.
This adds useless setup code to a users WORKSPACE file. The idea that we could add some transitive WORKSPACE dependencies is flawed without a working support for this in Bazel. Anything we add in that function later is a breaking change for users, in that they might call our py_repositories() before fetching rules_xx, and we already installed an incompatible rules_xx. Since adding anything here is a breaking change, we can always put it back later as a breaking change to the rules. However I would argue strongly that rules_python is too core in the dependency chain for it to *ever* grow transitive dependencies. Like rules_nodejs we should either vendor code we need privately or strip development-only dependencies from our distribution, and never suggest that users call a rules_python function that installs other starlark code.
alexeagle
force-pushed
the
kill_dependencies
branch
from
b868eb9 to
b5f0bce
Compare
alexeagle
added a commit
that referenced
this pull request
Jan 21, 2023
#846 introduced this dependency, and solved the broken examples by installing the dependency only for our own code. However, this was an inintentional breaking change for users, who may not have had bazel_skylib installed in their workspace. This effectively reverts #370. At that time we didn't want any dependencies, because managing them under Bazel's WORKSPACE semantics is so difficult for users. Now that bzlmod has reached General Availability in Bazel 6, such dependencies can be managed more easily. This also allows us to introduce bzl_library calls in our BUILD files, making it less brittle to ensure that users can generate docs for their rules/macros which load from rules_python.
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.
This adds useless setup code to a users WORKSPACE file.
The idea that we could add some transitive WORKSPACE dependencies is flawed without a working support for this in Bazel. Anything we add in that function later is a breaking change for users, in that they might call our py_repositories() before fetching rules_xx, and we already installed an incompatible rules_xx.
Since adding anything here is a breaking change, we can always put it back later as a breaking change to the rules.
However I would argue strongly that rules_python is too core in the dependency chain for it to ever grow transitive dependencies. Like rules_nodejs we should either vendor code we need privately or strip development-only dependencies from our distribution, and never suggest that users call a rules_python function that installs other starlark code.