Skip to content

Allow controlling the prefix added to repos/packages - #459

Merged
hrfuller merged 1 commit into
bazel-contrib:mainfrom
person142:controllable-prefix
Dec 22, 2021
Merged

Allow controlling the prefix added to repos/packages#459
hrfuller merged 1 commit into
bazel-contrib:mainfrom
person142:controllable-prefix

Conversation

@person142

@person142 person142 commented Apr 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread python/pip.bzl Outdated

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.

@person142 person142 Apr 22, 2021

Copy link
Copy Markdown
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
Comment thread python/pip.bzl Outdated
pip_install_dependencies()

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

@hrfuller hrfuller Apr 29, 2021 •
edited
Loading

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.

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.

@person142 person142 Apr 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

@person142 person142 Apr 29, 2021

Copy link
Copy Markdown
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...

@hrfuller hrfuller Dec 20, 2021

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.

@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.

@person142 person142 Dec 21, 2021

Copy link
Copy Markdown
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

thekyz commented Nov 3, 2021

Copy link
Copy Markdown

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

@thekyz

thekyz commented Nov 22, 2021

Copy link
Copy Markdown

@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

person142 commented Nov 30, 2021

Copy link
Copy Markdown
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

hrfuller commented Dec 20, 2021

Copy link
Copy Markdown
Contributor

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

@person142

person142 commented Dec 22, 2021

Copy link
Copy Markdown
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

@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 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

@thundergolfer thundergolfer Awaiting requested review from thundergolfer
1 more reviewer
@hrfuller hrfuller hrfuller approved these changes
Reviewers whose approvals may not affect merge requirements

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