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

refactor: don't load repo-phase objects from build-phase #2056

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

rickeylev
Copy link
Contributor

@rickeylev rickeylev commented Jul 11, 2024

As a general practice, the repo-phase and build-phase shouldn't load code from one
another because they can't use each other's objects. It can also result in confusing
behavior because the "starlark environment" is slightly different between the two phases.

Additionally, Google's version of Bazel essentially disables repo-phase objects, so
loading e.g. http_archive results in errors. This makes it more difficult to import
rules_python into Google, as we have to maintain patches to cut out the code (and thus
we spend more time trying to import the code than working on it).

@rickeylev rickeylev changed the title wip: factoring out build-phase loading repo-phase symbols refactor: don't load repo-phase objects from build-phase Jul 11, 2024
@rickeylev rickeylev marked this pull request as ready for review July 11, 2024 03:55
Copy link
Collaborator

@groodt groodt left a comment

Choose a reason for hiding this comment

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

LGTM

At canva we also ban the usage of these macros in our repo :) I'd be happy to see them gone entirely, but it doesn't seem viable at this stage because there is too much entrenched usage.

@rickeylev rickeylev added this pull request to the merge queue Jul 11, 2024
Merged via the queue into bazelbuild:main with commit 03854a2 Jul 11, 2024
4 checks passed
@rickeylev rickeylev deleted the split.pypi.repo.build.refs branch July 11, 2024 21:06
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