RFC: feat(pip_parse): Always generate pkg_aliases in the main pip repo

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.

[aignas]

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?

Currently the bzlmod aliases are only generated if we are using bzlmod. This causes an inconvenience, because the gazelle configuration has to be different.

Work towards #965.

What is the new behavior?

With this change, we could make a backwards incompatible change of the default pip repo convention implemented in #967 and support both bzlmod and non-bzlmod setups with the same gazelle config at the same time.

This is in essence implementing a very similar thing as to what was proposed in #814 and @rickeylev mentioned in there that having such aliases may lead to undefined behaviour, hence re-starting the discussion in the context of bzlmod.

cc: @f0rmiga, @alexeagle for extra opinions on gazelle and bzlmod future.

Does this PR introduce a breaking change?

• Yes
• No

[aignas]

Having given more thought about this and #814, I am thinking that having only a single way to access a particular library is something that we would lose here. If the user wants to test bzlmod and gazelle at the same time, then they could create aliases themselves (e.g. in a file //third_party/pip/BUILD.bazel:

load("@pip//:requirements.bzl", "requirement")
[
alias(
name=name,
actual=requirement(name),
visibility="//visibility:public",
)
fornamein [
# hardcoded list of packages found in requirements.in
]
]

Whilst regenerating BUILD.bazel files when switching to bzlmod is a cost, we do not sacrifice correctness.

If you agree with this, feel free to close this PR.

[groodt]

My 2c on the matter is that we need to rethink the requirement() implementation. It has added a lot of complexity and effectively is creating a sort of "closure" over all the external repositories.

I personally have wondered if we should try to remove the generated "requirement" with a plain old set of macros, similar to the approach taken in rules_jvm_external.

We could have a macro(s) outside a repository rule, that the user is then free to use for the following:

• requirement
• entry_point (console_scripts)
• scripts (legacy scripts)
• wheel files
• dist-info
• etc

In terms of providing gazelle with the relevant information it requires for mapping, we can expose a more meaningful set of metadata in the generated .bzl lockfile, for gazelle to use for discovery and mapping.

[aignas]

I don't think I have the full picture about the relation with rules_jvm_external, but I have to agree, that the macros originating from the requirements.bzl are rather hard to reason about. Thanks for the 2c.

I updated the #965 with a few more ideas about an alternative design of gazelle in bzlmod.

[aignas]

Closing in favour of #966, where we could include both of the change at the same time for easier review/more focused conversation.