Skip to content

Remove bzl_library from python/pip_install/BUILD so that skylib dep is not exposed to end-users - #430

Merged
thundergolfer merged 4 commits into
masterfrom
jonathon--issue-372
Mar 10, 2021
Merged

Remove bzl_library from python/pip_install/BUILD so that skylib dep is not exposed to end-users#430
thundergolfer merged 4 commits into
masterfrom
jonathon--issue-372

Conversation

@thundergolfer

Copy link
Copy Markdown

Addressing #372


PR Checklist

Please check if your PR fulfills the following requirements:

  • Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

expanding rules_python-0.1.0.tar.gz we see that /python/pip_install/BUILD has a load statement from @bazel_skylib.

Users weren't told to depend on that, nor did our WORKSPACE dependencies install such a thing.
Moreover until the new Bazel external deps story is landed, adding such a dependency is a nasty breaking change for users where they may have a hard time getting the right version of bazel_skylib to satisfy rules_python along with anything else that transitively depends on it.

Issue Number: #372

What is the new behavior?

/python/pip_install/BUILD no longer has a load statement from @bazel_skylib.

I've run ./update_docs.sh to check that this doesn't break anything, and it seems fine.

Does this PR introduce a breaking change?

  • Yes
  • No

@google-cla google-cla Bot added the cla: yes label Mar 7, 2021
@thundergolfer
thundergolfer requested review from alexeagle and hrfuller and removed request for brandjon and lberki March 7, 2021 12:20
@UebelAndre

Copy link
Copy Markdown
Contributor

Are there filegroups in place for users who want to generate documentation via stardoc?

@thundergolfer

Copy link
Copy Markdown
Author

@UebelAndre Can you elaborate, I'm not sure I know what you mean? Like you would use the filegroup to glob up a bunch of this workspaces .bzl files and use them as deps in another workspace's stardoc generation?

@UebelAndre

UebelAndre commented Mar 9, 2021

Copy link
Copy Markdown
Contributor

Correct, this is similar to what the releases of rules_nodejs does which also avoids the bazel_skylib dependency.

An example case of consuming that file group can be seen in @rules_rust//docs

@alexeagle

Copy link
Copy Markdown
Contributor

IMO the best answer is to use the file group in one of the integration tests, as a regression test that our published distro allows that use case. (Separate PR of course)

@hrfuller hrfuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thundergolfer
thundergolfer merged commit c30e05e into master Mar 10, 2021
@thundergolfer
thundergolfer deleted the jonathon--issue-372 branch March 10, 2021 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants