-
Notifications
You must be signed in to change notification settings - Fork 302
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
[#6664]: Invoke blaze mod
correctly
#6729
[#6664]: Invoke blaze mod
correctly
#6729
Conversation
2c28e4c
to
fb3aeef
Compare
@@ -38,6 +38,9 @@ | |||
|
|||
public class BlazeModRunnerImpl extends BlazeModRunner { | |||
|
|||
private static final String DUMP_REPO_MAPPING = "dump_repo_mapping"; | |||
private static final String ROOT_WORKSPACE = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously it was "workspace", not an empty string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the context there: #6664 (comment)
Could you summarize that in a code comment?
@@ -83,6 +83,9 @@ public ListenableFuture<ExternalWorkspaceData> getExternalWorkspaceData( | |||
return Futures.immediateFuture(ExternalWorkspaceData.EMPTY); | |||
} | |||
|
|||
// TODO: when 8.0.0 is released add this conditionally only when it's disabled inside `starLarkSemantics` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you also explain the need for this flag in a comment?
fb3aeef
to
23e08ae
Compare
@agluszak updated comments and moved the flag patch to the execution place (to be near the comment) |
* repository map we would invoke it like {@code bazel mod dump_repo_mapping ""}. | ||
* <p /> | ||
* Additionally the flag {@code --enable_workspace} needs to be off for this to work. The flag is default | ||
* off in bazel 8.0.0 but it is on between 7.1.0 and 8.0.0. So we need to also pass this along in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this flag supported by Bazel <7.1.0? If not, doesn't it break Bazel 6 and 5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not get called at all before 7.1.0 since the dump command itself didn't exist before.
Checklist
Discussion thread for this change
Issue number: #6664
Description of this change
Invoke
blaze mod dump_repo_mapping
correctly