-
Notifications
You must be signed in to change notification settings - Fork 303
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
Resolve external paths to their workspaces #4063
Conversation
Please see bazelbuild#2766 for context
String regexSep = Pattern.quote(f.separator); | ||
// Workspace name defaults to __main__ per DEFAULT_REPOSITORY_DIRECTORY in | ||
// https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java | ||
// Valid workspace name regex copied from LEGAL_WORKSPACE_NAME 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.
If you all already know of a better way of referencing Bazel source from within the IntelliJ plugin, please say something. Ideally, it'd be good to reference LEGAL_WORKSPACE_NAME instead of duplicating the regex. (But of course, way better to fix the external workspaces then to block on this.)
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 do not think there is a way to use LEGAL_WORKSPACE_NAME
here.
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.
👍🏻 Sounds like we proceed then. Figured I should ask, though :)
Reassigned to @tpasternak. I wonder if it will be safer to guard this change with an experiment to be gradually released, just to allow the users to opt-out of it in case it breaks anything. |
Thanks for taking a look, and looking forward to working with you, @tpasternak. [Totally up to you guys on flagging of course. But I thought I should note that this bug has been pretty widely fixed in the other Bazel IDE adaptors w/o incident. More in the issue, linked up top.] |
Gentle bump @tpasternak, since it's been a good while and y'all explicitly asked me to post this one? |
Sure, going to look at it, sorry for the delay |
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.
So I think the diagnosis and the solution is correct.
The only thing I would prefer to avoid is the regex-based path replacement. What do you think about adding a new outputBase
constructor parameter to ExecutionRootPathResolver and just calling path.getRootedAt(outputBase)
instead of path.getRootedAt(executionRoot)
?
This would IMO make it more consistent with other parts of code, where we make use of outputBase as well
…putBase) Based on tpasternak's good feedback. Please see bazelbuild#2766 and bazelbuild#4063 for context.
…putBase) Based on tpasternak's good feedback. Please see bazelbuild#2766 and bazelbuild#4063 for context.
@tpasternak, that's definitely better. Done. Thanks for great feedback. Some more minor notes:
Thanks! |
…putBase) Based on tpasternak's good feedback. Please see bazelbuild#2766 and bazelbuild#4063 for context.
// Build artifacts accumulate under the execution root, independent of symlink settings | ||
return path.getFileRootedAt(executionRoot); | ||
} | ||
if (firstPathComponent.equals("external")) { // In external 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.
Hey, I'm not sure if I get it correctly, why do we need to modify resolveExecutionRootPath method? Isn't it enough just to modify resolveToIncludeDirectories
? And is it possible that ExecutionRootPath starts from "external"?
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.
Hey hey! Thanks for taking another look, @tpasternak.
I'd think we'd need to modify execution root paths--and not just directories--because one might try to browse to an external file path. More generally, those execroot paths are unstable for all files, not just directories, so I'd think we'd always want to point to the stable paths, lest they disappear under the editor when a build gets run that uses different external workspaces. This ended up being important for Xcode/VSCode, anyway. I think this is a case we want to defend against. (Unless I'm misunderstanding your question; you all are the experts, immersed in this day-to-day.)
Thank you @cpsauer ! |
Yay! Thank you, @tpasternak! |
Please see #2766 for context.
@sgowroji offered to shepherd.