Skip to content
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

Turn //python/private into a package. #555

Merged
merged 3 commits into from
Oct 26, 2021
Merged

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Oct 22, 2021

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?

See #557

This turns ./python/private into a Bazel package

Issue Number: #557

What is the new behavior?

This change adds the //python/private package

Instead of loading //python:private/reexports.bzl, users will now need to load //python/private:reexports.bzl

Does this PR introduce a breaking change?

This is not marked as a breaking change, because loads under the /private folder were never public API.

  • Yes
  • No

@google-cla google-cla bot added the cla: yes label Oct 22, 2021
@UebelAndre UebelAndre mentioned this pull request Oct 22, 2021
13 tasks
@UebelAndre UebelAndre force-pushed the private branch 4 times, most recently from 7e8dbc5 to 77e4a70 Compare October 22, 2021 15:47
@UebelAndre UebelAndre marked this pull request as ready for review October 22, 2021 16:02
@UebelAndre
Copy link
Contributor Author

This PR includes the changes in #556

@alexeagle
Copy link
Collaborator

I'm not clear what's the benefit of making another package under the private/ folder vs. what we have now?

@UebelAndre
Copy link
Contributor Author

I'm not clear what's the benefit of making another package under the private/ folder vs. what we have now?

Please see #554 (comment)

@alexeagle
Copy link
Collaborator

https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#bzl-visibility doesn't mention packages, but rather directories.

However I have a few minutes to find out for sure:

alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ touch WORKSPACE
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ mkdir dir
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ mkdir pkg
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ mkdir dir/private
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ mkdir pkg/private
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ echo "A = []" > dir/private/a.bzl
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ echo "B = []" > pkg/private/b.bzl
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ echo 'load("//dir:private/a.bzl", "A")' >> BUILD
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ echo 'load("//pkg/private:b.bzl", "B")' >> BUILD
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ touch dir/BUILD
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ touch pkg/private/BUILD
alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ echo 'C = A + B' >> BUILD

alexeagle@system76-pc:/tmp/tmp.TN7MzAjbHR$ buildifier --lint=warn BUILD 
BUILD:2: bzl-visibility: Module "//pkg/private:b.bzl" can only be loaded from files located inside "//pkg", not from "//BUILD". (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#bzl-visibility)
BUILD:3: unused-variable: Variable "C" is unused. Please remove it.
To disable the warning, add '@unused' in a comment. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#unused-variable)

So I think this means you're right, if private is a directory but not a nested package, users won't get a warning about using private APIs.

Confirmed also that adding this line to one of our examples

load("@rules_python//python:private/reexports.bzl", "internal_PyInfo")

doesn't cause a bzl-visibility lint.

So there's actually an issue being fixed here.

@alexeagle
Copy link
Collaborator

Filed #557 which is the explanation I was looking for :)

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

thanks!

@alexeagle alexeagle merged commit 2c96d82 into bazelbuild:main Oct 26, 2021
@UebelAndre UebelAndre deleted the private branch November 28, 2021 15:35
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.

2 participants