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

incompatible_remove_local_resources #10536

Closed
susinmotion opened this issue Jan 7, 2020 · 17 comments
Closed

incompatible_remove_local_resources #10536

susinmotion opened this issue Jan 7, 2020 · 17 comments
Labels
incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) team-Local-Exec Issues and PRs for the Execution (Local) team

Comments

@susinmotion
Copy link
Contributor

susinmotion commented Jan 7, 2020

--local_resources will be deprecated in favor of --local_ram_resources and --local_cpu_resources.

--incompatible_remove_local_resources will become true by default, which will cause breakages if --local_resources is used. Please use --local_ram_resources and --local_cpu_resources instead of --local_resources.

Migration has been in progress since March 2019. --local_resources should be fully deprecated within two releases.

@susinmotion susinmotion added incompatible-change Incompatible/breaking change breaking-change-3.0 labels Jan 7, 2020
@tetromino tetromino added P2 We'll consider working on this in future. (Assignee optional) team-Local-Exec Issues and PRs for the Execution (Local) team labels Jan 7, 2020
@dslomov
Copy link
Contributor

dslomov commented Feb 3, 2020

Since when is --incompatible_remove_local_resources available? Which release it is in? This flag does not have any migration-* labels (please follow Guide for rolling out breaking changes for incompatible changes)

@susinmotion
Copy link
Contributor Author

It came out in March 2019. The instructions are a little confusing wrt the process for adding migration-* labels. Am I supposed to add this? Or is it part of the release manager process?

@dslomov
Copy link
Contributor

dslomov commented Feb 3, 2020

Per https://www.bazel.build/breaking-changes-guide.html#labels, the change author is supposed to add migration-ready label once the flag is in, and the release manager will then add appropriate 'migration-*' labels.

Since this didn't happen for this flag (in fact, the bug for the change has only been added a month ago) our users have had no warning that it is coming.

Let me add some labels that will enable testing for the users.

@susinmotion
Copy link
Contributor Author

Thank you.
So, I should add migration-ready? Or is that just to indicate to the release manager to add the other tags?
Anything else that needs to happen?

@dslomov
Copy link
Contributor

dslomov commented Feb 3, 2020

Since the flag is in and we have added "migration-2.0" and "migration-2.1" and watch https://buildkite.com/bazel/bazelisk-plus-incompatible-flags to assess how badly we break the world.

If we really want to flip this flag in March, it might also be a good idea to send a mail to bazel-discuss@ and see if people complain.

@dslomov
Copy link
Contributor

dslomov commented Feb 6, 2020

It looks like only rules_nodejs are affected. @susinmotion could you file a bug on https://github.com/bazelbuild/rules_nodejs to address this?

@susinmotion
Copy link
Contributor Author

Done. bazel-contrib/rules_nodejs#1614

@susinmotion
Copy link
Contributor Author

Relatedly, --ram_utilization_factor has been a no-op for ~a year (it's replaced by --local_ram_resources), and there's been a lot of back and forth about whether we need to guard its removal behind a flag, resulting in it still being around.

I just added you as a reviewer on that change, so if you could look at the comments and weigh in there, that would be great.

@dslomov
Copy link
Contributor

dslomov commented Feb 7, 2020

Given limited breakage, I think it is ok to flip this in 3.0

@susinmotion
Copy link
Contributor Author

Great. So I should wait until the release window for 3.0, and then submit?

@dslomov
Copy link
Contributor

dslomov commented Feb 7, 2020

You can submit the flag flip after 2.2 is cut (see #10133

@alexeagle
Copy link
Contributor

I noticed that https://docs.bazel.build/versions/2.0.0/command-line-reference.html#flag--local_resources doesn't mention the deprecation

@laurentlb
Copy link
Contributor

@susinmotion: can you please update the documentation?
Even at head, the flag is still here, documented, and not deprecated: https://docs.bazel.build/versions/master/command-line-reference.html#flag--local_resources

@susinmotion
Copy link
Contributor Author

This is autogenerated, correct? I just sent you a change updating the help text for the flag, and hopefully that'll show up here.

bazel-io pushed a commit that referenced this issue Feb 13, 2020
@laurentlb
Copy link
Contributor

We're supposed to cut Bazel 3.0 now. Was this flag flipped?

@susinmotion
Copy link
Contributor Author

susinmotion commented Mar 3, 2020

Ack, I reversed the comments on the two flags.
I just submitted this change, and am preparing a change for remove ram utilization factor.

@katre
Copy link
Member

katre commented Mar 4, 2020

This was flipped in 765d3c1.

luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Development

No branches or pull requests

7 participants