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

Update python_info aspect to accommodate Python targets that only have rules_python providers #210

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

mnoah1
Copy link
Contributor

@mnoah1 mnoah1 commented Jan 22, 2025

As the Python rules are gradually phasing out use of the builtin PyInfo provider in favor of the ones from rules_python, there could be cases where a repository is on a Bazel version lower than 8, but the built in PyInfo is still missing from a given target.

In our repo's case, we are in the process of migrating to rules_py, which uses the rules_python providers only (aspect-build/rules_py#311), and not the built in ones.

This change adds support for rules_python providers in the aspect for Bazel versions less than 8:

  • Bazel <8 - this adds an extra check rules_python in the repo's external rules. The load statement will only run if present, and when this is the case, the rules_python providers will be considered as a secondary option to the builtin ones.
  • Bazel 8 - rules_python providers only [existing behavior]

@mnoah1 mnoah1 force-pushed the rules_python_providers branch from 91c7c77 to cbcfcbe Compare January 24, 2025 19:11
@mnoah1
Copy link
Contributor Author

mnoah1 commented Jan 24, 2025

It looks like all the e2e tests pass with this change, but I'm seeing a failure in the [analysis] Qodana android-forked-bazel-rules-project step with error: found 'project contains no modules' in the build log. I don't think it seems related to my change but I'm not quite sure - could you help me verify?

@mnoah1 mnoah1 requested a review from agluszak January 24, 2025 20:27
@mnoah1
Copy link
Contributor Author

mnoah1 commented Jan 31, 2025

Hi @agluszak - could you give this one another look when you have a chance? Thanks.

@jastice jastice assigned sellophane and agluszak and unassigned sellophane Feb 6, 2025
@JetBrainsBazelBot
Copy link
Collaborator

Hi, we've added a new test - could you please merge main for it to pass?
regarding [analysis] Qodana android-forked-bazel-rules-project - we are investigating, but it seems your change indeed breaks something in one of our android project e2e test

@mnoah1
Copy link
Contributor Author

mnoah1 commented Feb 7, 2025

could you please merge main for it to pass?

Updated - looks like the new one is passing.

regarding [analysis] Qodana android-forked-bazel-rules-project - we are investigating, but it seems your change indeed breaks something in one of our android project e2e test

Thanks - let me know if you find any more specifics (I was able to easily run the other e2e tests locally, but seems the Qodana ones require a token and are a bit harder for me to debug myself).

@JetBrainsBazelBot
Copy link
Collaborator

we'll get back to you monday on the qodana test

@gottagofaster236
Copy link
Member

gottagofaster236 commented Feb 7, 2025

This is the log from the failing Android test. I'm not 100% sure why it's failing but the test project indeed does not have a dependency on rules_python

2025-02-07T12:42:51.793Z [13336] INFO - �[31m�[1mERROR: �[0mat /root/.cache/bazel/_bazel_idea/d1aa40fbc8d41d43ca12eb8239db2224/external/bazelbsp_aspect/aspects/core.bzl:3:6: at /root/.cache/bazel/_bazel_idea/d1aa40fbc8d41d43ca12eb8239db2224/external/bazelbsp_aspect/aspects/extensions.bzl:3:6: at /root/.cache/bazel/_bazel_idea/d1aa40fbc8d41d43ca12eb8239db2224/external/bazelbsp_aspect/aspects/rules/python/python_info.bzl:4:6: Unable to find package for @@rules_python//python:defs.bzl: The repository '@@rules_python' could not be resolved: Repository '@@rules_python' is not defined.

@mnoah1 mnoah1 force-pushed the rules_python_providers branch from 6de6ad1 to 1e4087c Compare February 11, 2025 17:46
@mnoah1 mnoah1 marked this pull request as draft February 11, 2025 17:46
@mnoah1 mnoah1 marked this pull request as ready for review February 11, 2025 17:46
@mnoah1 mnoah1 changed the title Update python_info aspect to accommodate Python targets that only have rules_python providers [work in progress] Update python_info aspect to accommodate Python targets that only have rules_python providers Feb 11, 2025
@mnoah1 mnoah1 force-pushed the rules_python_providers branch from 1e4087c to a59b312 Compare February 11, 2025 18:21
@mnoah1 mnoah1 changed the title [work in progress] Update python_info aspect to accommodate Python targets that only have rules_python providers Update python_info aspect to accommodate Python targets that only have rules_python providers Feb 11, 2025
@mnoah1
Copy link
Contributor Author

mnoah1 commented Feb 11, 2025

This is now updated to better handle possible scenarios, and all checks are now passing. This is ready for another look when you have a chance.

@mnoah1
Copy link
Contributor Author

mnoah1 commented Feb 21, 2025

@agluszak @jastice @gottagofaster236 just wanted to follow up on this.

@xuansontrinh
Copy link
Contributor

@mnoah1 the PR LGTM.
Could you please resolve the conflict with the readme file and then we can merge it?

@xuansontrinh xuansontrinh merged commit c889600 into JetBrains:main Feb 21, 2025
1 of 27 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.

6 participants