Fix recursive use of maybe in pip_repository rules.

[UebelAndre]

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?

• 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

[hrfuller]

FWIW bazel recommends not using native.existing_rule.

If possible, avoid using this function. It makes BUILD files brittle and order-dependent.

[thundergolfer]

Also

Note that the existing_rule(s) functions are poorly defined, have design issues, can lead to performance and maintenance issues. So we don't really want to encourage their use.

It seems rules_go has for many years used native.existing_rules to have a 'fake maybe' https://github.com/bazelbuild/rules_go/pull/782/files.

[UebelAndre]

I think that note is describing common issues in WORKSPACE management. I don't understand why maybe would be ok but not the native functions. One argument might be because maybe can't be used recursively but I think this situation is notably different in that users should be able to treat the pip_repository rules as normal repository rules, but there's some macro magic involved to inject the necessary dependencies. I think the use of these native functions here is fine but I'm also happy to learn better practices 😄

[thundergolfer]

So you're attempting to do:

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

is that right? What's the use case?

[UebelAndre]

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.

[UebelAndre]

@thundergolfer @hrfuller friendly ping 😄

[UebelAndre]

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

[thundergolfer]

@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

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]

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

[thundergolfer]

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?

[UebelAndre]

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.

[UebelAndre]

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.

[alexeagle]

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.

[UebelAndre]

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: #531 (comment)