Skip to content
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.

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

chore: remove internal usage of deprecated py_binary ad py_test #2569

Merged
merged 18 commits into from
Jan 22, 2025

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jan 21, 2025

This goes together with #2565 to remove the internal usage of the deprecated
symbols. This also fixes the compile_pip_requirements symbol to print the
correct deprecation message.

Builds on top of 611eda8

The example message that would be printed is as follows:

The 'py_test' symbol in '@+python+python_3_11//:defs.bzl'
is deprecated. It is an alias to the regular rule; use it directly instead:

load("@rules_python//python:py_test.bzl", "py_test")

py_test(
    name = "versioned_py_test",
    srcs = ["dummy.py"],
    main = "dummy.py",
    python_version = "3.11.11",
)

@aignas aignas requested a review from rickeylev as a code owner January 21, 2025 02:24
@aignas
Copy link
Collaborator Author

aignas commented Jan 21, 2025

This is also deprecating the symbols in the pythons_hub and the other parts as was suggested in #2565. If this is not (yet) desirable, please comment on this PR. I personally think that we might be able to do this, but I am not sure if there are no problems (e.g. with using 3.10 in non-root modules).

If we want to start printing deprecation messages in 3.10 or add a flag that is controlling this behaviour via rules_python_internal or some other method, I am happy to amend to this PR.

Maybe the with_deprecation symbol should be in //python/private:with_deprecacion?

I ran out of time today, so that is it for today.

@aignas
Copy link
Collaborator Author

aignas commented Jan 21, 2025

This now only warns if RULES_PYTHON_DEPRECATION_WARNINGS is 1, which defaults to 0. I am fine with defaulting it to 1 if we want to default to a more noisy default.

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Just nits/style, otherwise LGTM

docs/environment-variables.md Outdated Show resolved Hide resolved
python/config_settings/transition.bzl Outdated Show resolved Hide resolved
python/private/internal_config_repo.bzl Outdated Show resolved Hide resolved
@rickeylev
Copy link
Collaborator

This is great, thank you! I didn't realized I missed so many other spots, sorry about that.

re: deprecating pythons_hub symbols: Yes, deprecating those also makes sense

re: move deprecation helper elsewhere: yes, I agree

re: environment variable: Thanks for creating this. I like this idea a lot.

@rickeylev
Copy link
Collaborator

addendum: I've approved, but not queued for merge, in case you want to move with_deprecated elsewhere before merging. I'm OK with that being done in a separate PR, too.

@aignas aignas enabled auto-merge January 22, 2025 00:38
@aignas
Copy link
Collaborator Author

aignas commented Jan 22, 2025

OK, extra changes that I needed to do:

  • Change the usage of rctx.os.environ to rctx.getenv via repo_utils in the config repo.
  • Move the deprecation utility to a separate file.

I think this makes the change much more robust and we have a single place for the deprecated symbols.

If we wanted we could add a buildozer command for the migration, but I think we can merge as is right now.

@aignas aignas added this pull request to the merge queue Jan 22, 2025
Merged via the queue into bazelbuild:main with commit 188598a Jan 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants