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.

Sign up for GitHub

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

Jump to bottom

Lazy expansion of symbolic macros #23852

Open
tetromino opened this issue Oct 2, 2024 · 4 comments
Open

Lazy expansion of symbolic macros #23852

tetromino opened this issue Oct 2, 2024 · 4 comments
Assignees
@brandjon
Labels
P1 I'll work on this now. (Assignee required) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: feature request

Comments

@tetromino
Copy link
Contributor

Tracking bug for implementing lazy expansion of symbolic macros (the eager version of which was implemented in #19922).

Tentatively targeting Bazel 8.1.

@brandjon @susinmotion FYI
@tetromino tetromino added type: feature request P1 I'll work on this now. (Assignee required) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob labels Oct 2, 2024
@brandjon
Copy link
Member

brandjon commented Oct 3, 2024

@meteorcloudy @meisterT This was slated for 8.0 but I think the timing's too tight. Certainly it's not making the cut date of Monday, but I was entertaining the idea of cherrypicking it in over the following weeks. Can you comment on the feasibility of that process vs backporting to a later 8.x? Is it the same amount of work but one inconveniences a release manager and the other doesn't?

copybara-service bot pushed a commit that referenced this issue Oct 4, 2024
@tetromino @copybara-github
Prohibit calls to native.environment() in symbolic macros
5c82594 
environment_group() is a special non-rule callable which can only be
invoked at top level in a BUILD file (there is no
native.environment_group()) and which creates a special target that refers
to environment rule targets via unresolved labels.

This means that when we allow lazy macro expansion, if an environment
target is declared in the scope of a macro, during constraint enforcement
the environment group might or might not find the environment target
depending on whether or not the symbolic macro happened to be expanded.

By far the simplest solution is to prohibit calls to native.environment()
in symbolic macros.

Working towards #23852

PiperOrigin-RevId: 682141401
Change-Id: Ic32e58ade1db01e1de53e747c43544a4327dbc5e
@meisterT
Copy link
Member

meisterT commented Oct 4, 2024

IMO, it mostly depends on how well isolated the code is and how risky it is that it would break other functionality.

@Wyverald
Copy link
Member

Wyverald commented Oct 4, 2024

The difference between cherry-picking back to 8.0 vs 8.1 is just the timing -- cherry-picking to 8.0 post rc1 drags out further rc's. I'd like to avoid any non-regression fixes past rc1 to avoid slipping like past releases. Major releases already tend to slip a lot and it wears on release managers.

@brandjon
Copy link
Member

brandjon commented Oct 7, 2024

Ok, thanks, that confirms that we won't want to try to fit laziness into 8.0. Once the other macro-related 8.0 blockers are resolved we'll implement laziness and see what it takes to backport into 8.x (x>0).

@qodo-merge-pro qodo-merge-pro bot mentioned this issue Nov 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brandjon brandjon

Labels
P1 I'll work on this now. (Assignee required) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tetromino @meisterT @Wyverald @brandjon