rules_python

rules_python copied to clipboard

PR Checklist

Please check if your PR fulfills the following requirements:

• [ ] 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?

• [x] 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?

Users trying to wrap pip_parse or pip_install with maybe will run into a failure for attempting to use the rule recursively. This can be fixed by avoiding maybe in these macro repository rules.

Issue Number: N/A

What is the new behavior?

maybe can now be used with pip_parse and pip_install.

Does this PR introduce a breaking change?

• [ ] Yes
• [ ] No

Other information

So you're attempting to do:

maybe(
   pip_install,
   "<name>",
   **kwargs,
 )

is that right? What's the use case?

So you're attempting to do:

maybe(
   pip_install,
   "<name>",
   **kwargs,
 )

is that right? What's the use case?

I have a repository I'm sharing between different workspaces which uses python. In each workspace loading it, I want the pip_parse repository which produces the dependencies to be used in the shared repository. There's a macro which loads all dependencies including the pip_parse repository and pip_parse is the only one that can't be wrapped via maybe. I think that it should be able to be.

@thundergolfer @hrfuller friendly ping 😄

@thundergolfer @hrfuller another ping, would love to get this in 🙏

@UebelAndre Still feel uneasy about this.

I went on another code search to learn about the use of native.existing_rule* in the wild and here's you ripping out native.existing_rule in cargo-raze 😄 https://github.com/google/cargo-raze/pull/207

If the pip_parse wasn't non-optionally grabbing deps, would you avoid recursive maybe?

# pip.bzl
    ...
    # Just in case our dependencies weren't already fetched
    pip_install_dependencies()

https://github.com/bazelbuild/rules_python/blob/main/python/pip.bzl#L183

@UebelAndre Still feel uneasy about this.

I went on another code search to learn about the use of native.existing_rule* in the wild and here's you ripping out native.existing_rule in cargo-raze 😄 google/cargo-raze#207

This is a totally different scenario since cargo-raze isn't trying to pass macros which internally define other repositories as one repository rule.

If the pip_parse wasn't non-optionally grabbing deps, would you avoid recursive maybe?

# pip.bzl
    ...
    # Just in case our dependencies weren't already fetched
    pip_install_dependencies()

https://github.com/bazelbuild/rules_python/blob/main/python/pip.bzl#L183

My only goal is to be able to use maybe with pip_parse and pip_install. The pip_repository dependencies could be moved into a separate macro but that be a breaking change where native.existing_rules would not be. Can you elaborate on your concerns about native.existing_rules?

Can you elaborate on...

It's mostly just https://github.com/bazelbuild/rules_python/pull/612#discussion_r796505691

Unless we're solving a critical case, opting into functionality the Bazel devs have explicitly advised against seems bad practice. So while you do have a valid case, solving for it might be 'one step forward two steps back'

If you opted out of the pip_parse macro and used pip_repository directly, wouldn't you have things working?

Can you elaborate on...

It's mostly just #612 (comment)

Unless we're solving a critical case, opting into functionality the Bazel devs have explicitly advised against seems bad practice. So while you do have a valid case, solving for it might be 'one step forward two steps back'

If you opted out of the pip_parse macro and used pip_repository directly, wouldn't you have things working?

I don't think users should need to directly use pip_repository. I really don't think the notes about the usage for native.existing_rules applied to rule authors. Plus, this was written way back in 2019 so things may have stabilized since then. Ultimately, I feel pip_parse and pip_install are promoted as repository rules (or are at least assumed to be ones at first glance) and would like to be able to use them in this manner. I see this as a very minor change that respects bazel best practices more than it might "violate" anything.

Thinking more about pip_repository. I think the thing I ultimately want is for rules_python to provide a rules_python_dependencies macro in accordance with bazel-contrib and https://docs.bazel.build/versions/main/skylark/deploying.html instead of trying to hide them in macros. That way pip_parse and pip_install could be updated to be regular repository rules. But if that's not desirable, then I think this change is an acceptable middle ground.

As one mitigation here, in #531 I'm adding stardoc for the pip_repository rule so it's better documented that you don't have to use the two wrapper macros.

As one mitigation here, in #531 I'm adding stardoc for the pip_repository rule so it's better documented that you don't have to use the two wrapper macros.

@alexeagle I think this change is still useful. I commented on that PR: https://github.com/bazelbuild/rules_python/pull/531#issuecomment-1106734325