Skip to content

Conversation

@Yannic
Copy link
Contributor

@Yannic Yannic commented Dec 6, 2021

Host transitions are problematic where the host and exec environment are
different (e.g., with Remote Execution). This change adds an incompatible
flag that fails analysis if Starlark rules use cfg = "host".

Migrating is pretty straigt forward (replace host with exec) and
should be automatable by buildifier (although I haven't tried to do so).

Host transitions are problematic where the host and exec environment are
different (e.g., with Remote Execution). This change adds an incompatible
flag that fails analysis if Starlark rules use `cfg = "host"`.

Migrating is pretty straigt forward (replace `host` with `exec`) and
should be automatable by buildifier (although I haven't tried to do so).
@google-cla google-cla bot added the cla: yes label Dec 6, 2021
@Yannic
Copy link
Contributor Author

Yannic commented Dec 6, 2021

@katre and/or @gregestren PTAL

@katre katre requested a review from comius December 6, 2021 14:52
@katre
Copy link
Collaborator

katre commented Dec 6, 2021

@comius is actually working on something similar, so I'm assigning this to him so the two of you can discuss.

thii added a commit to thii/rules_apple that referenced this pull request Dec 6, 2021
This doesn't really matter now because all tools here are just Bash and
Python scripts, but could prevent problems later when an incompatible
flag to disable cfg="host" from Starlark lands.

bazelbuild/bazel#14383
thii added a commit to bazelbuild/rules_apple that referenced this pull request Dec 7, 2021
This doesn't really matter now because all tools here are just Bash and
Python scripts, but could prevent problems later when an incompatible
flag to disable cfg="host" from Starlark lands.

bazelbuild/bazel#14383
@comius
Copy link
Contributor

comius commented Dec 15, 2021

cc @susinmotion

I would attempt the more aggressive path first, where we just change all "host" transitions to "exec". This would reduce the churn substantially.

We're already fixing some issues that happen with that plan (within Bazel).

If this plan fails, this PR becomes viable alternative.

@Yannic
Copy link
Contributor Author

Yannic commented Dec 21, 2021

In my experiments, it seemed like tools build in exec config do get --host_* flags, so that plan doesn't look obviously broken (at least for local exec, didn't try remote exec yet but I should probably do that). SGTM

I think we should probably still make cfg="host" an error eventually even after cfg="host" == cfg="exec" (similar to what was done to cfg="data" which first became a no-op and was the removed), but we can handle that at a later point.

Should I send the documentation change in a separate PR? That seems useful regardless of whether we add the flag or not.

@gregestren gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Feb 1, 2022
@gregestren
Copy link
Contributor

@susinmotion 's been focusing more on this recently. @susinmotion - do you have recommendations for @Yannic 's latest comment?

@susinmotion
Copy link
Contributor

Cool, thanks for thinking about this. A separate PR for documentation sounds great--we're really close to making this change.

I'm torn about making cfg="host" an error after the migration. On the one hand, it will make people's rules less confusing in the long term, since they'll say "exec" when they mean "exec". On the other hand, it causes breakages with things that aren't actually a problem, and that's a bummer for users. Can we do some kind of migration script here instead?

@Yannic
Copy link
Contributor Author

Yannic commented Feb 21, 2022

@susinmotion thanks for the update!

Can we do some kind of migration script here instead?

Yes! Added a buildifier warning/fix for this: bazelbuild/buildtools#1053

@comius
Copy link
Contributor

comius commented Mar 23, 2022

Hey, can you resolve the conflicts? Host transition has been removed now, so a flag to completely disable it is useful.

Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

Please resolve the conflicts.

@comius
Copy link
Contributor

comius commented Apr 11, 2022

Please resolve the conflicts.

Friendly ping.

@Yannic
Copy link
Contributor Author

Yannic commented Apr 11, 2022

@comius Updated, PTAL

@bazel-io bazel-io closed this in 6464f1c Apr 12, 2022
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 12, 2022
@ckolli5
Copy link

ckolli5 commented Apr 13, 2022

@bazel-io fork 5.2.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 13, 2022
ckolli5 added a commit that referenced this pull request May 9, 2022
Host transitions are problematic where the host and exec environment are
different (e.g., with Remote Execution). This change adds an incompatible
flag that fails analysis if Starlark rules use `cfg = "host"`.

Migrating is pretty straigt forward (replace `host` with `exec`) and
should be automatable by buildifier (although I haven't tried to do so).

Closes #14383.

PiperOrigin-RevId: 441253060

Co-authored-by: Yannic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes team-Configurability platforms, toolchains, cquery, select(), config transitions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants