Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Allow controlling the prefix added to repos/packages #459

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 22, 2021
Merged

Allow controlling the prefix added to repos/packages #459

merged 1 commit into from
Dec 22, 2021

Conversation

person142
Copy link
Contributor

@person142 person142 commented Apr 19, 2021 •
edited
Loading

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?

Issue Number:

Currently the prefix added to the generated repos for pip_parse is hard-coded to <pip-parse-name>_pypi__ and the prefix added to the generated packages for pip_install is hard-coded to pypi__.

What is the new behavior?

Allow the user to control the prefix. You still need some prefix to avoid #414 (comment), but for pip_parse in particular you can make the external repository names more memorable:

$ cat requirements.txt
attrs==20.3.0
$ cat WORKSPACE
local_repository(
    name = "rules_python",
    path = "../rules_python",
)

load("@rules_python//python:pip.bzl", "pip_install", "pip_parse")

pip_parse(
    name = "deps",
    requirements_lock = "//:requirements.txt",
    repo_prefix="pypi_",
)

load("@deps//:requirements.bzl", "install_deps")
install_deps()
$ bazelisk build @pypi_attrs//:pkg
INFO: Analyzed target @pypi_attrs//:pkg (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target @pypi_attrs//:pkg up-to-date (nothing to build)
INFO: Elapsed time: 0.095s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

None

@google-cla google-cla bot added the cla: yes label Apr 19, 2021
@person142 person142 mentioned this pull request Apr 19, 2021
Closed
@person142 person142 force-pushed the controllable-prefix branch 3 times, most recently from a8c8fdf to e4c8663 Compare April 22, 2021 03:50
@person142 person142 marked this pull request as ready for review April 22, 2021 03:53
person142
person142 commented Apr 22, 2021
View reviewed changes
python/pip.bzl Outdated
**kwargs
)

def pip_parse(requirements_lock, name = "pip_parsed_deps", **kwargs):
def pip_parse(requirements_lock, name = "pip_parsed_deps", repo_prefix = None, **kwargs):
"""Generate external repositories for each entry in a requirements lockfile.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buildifier got unhappy that there was no docstring here.

hrfuller
hrfuller reviewed Apr 29, 2021
View reviewed changes
python/pip.bzl Outdated
# Just in case our dependencies weren't already fetched
pip_install_dependencies()

if repo_prefix == None:
repo_prefix = "{name}_pypi__".format(name = name)
Copy link
Contributor

@hrfuller hrfuller Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Without the parent repo name in the repo_prefix these repositories won't be unique, and if users have multiple instances of pip_parse with the same repo_prefix they could collide. I'm not sure I'm in favor of allowing that.

Copy link
Contributor Author

@person142 person142 Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We could move to a pattern like <repo-name>_<pkg-name>-my biggest objective is to have short, memorable names that are easy to refer to by label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i.e. most of all I want to solve #414, which just so happens to be easier in the pip_parse case than the pip_install case...

Copy link
Contributor

Choose a reason for hiding this comment

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

@person142 if we can make these repo names unique using <repo-name>_<pkg-name> Where is the PEP503 canonical package name I think thats a good soln.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Should have some time to make the change today.

@thekyz
Copy link

thekyz commented Nov 3, 2021

What's the status of this PR? In monorepos with a lot of packages controlling that prefix can be a life saver.

@thekyz
Copy link

thekyz commented Nov 22, 2021

@thundergolfer would you be willing to review this if I updated the PR? We currently have a bad alternative to this as a patch in our repo so I'd prefer to have it clean from mainstream.

@person142
Copy link
Contributor Author

I can update this if desired/needed-just don't have the bandwidth right now to rally support for a particular solution.

@hrfuller
Copy link
Contributor

@thekyz @person142 taking another look at this today. If we can resolve the naming collision issue I'll be happy to ship.

@person142
Copy link
Contributor Author

@hrfuller refactored so that the repos for pip_parse are now always <name>_<package-name>.

@person142 person142 requested a review from hrfuller December 22, 2021 18:24
hrfuller
hrfuller approved these changes Dec 22, 2021
View reviewed changes
Copy link
Contributor

@hrfuller hrfuller left a comment

Choose a reason for hiding this comment

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

LGTM thanks @person142

@hrfuller hrfuller merged commit beed9a2 into bazel-contrib:main Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hrfuller hrfuller hrfuller approved these changes

@thundergolfer thundergolfer Awaiting requested review from thundergolfer

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow customizing DEFAULT_PACKAGE_PREFIX in pip_parse
3 participants
@person142 @thekyz @hrfuller
Morty Proxy This is a proxified and sanitized view of the page, visit original site.