Store the package and name of a Label as strings rather than paths, to avoid occurrence of backslashes on Windows #7271
Conversation
odisseus
requested review from
mai93,
ujohnny,
LeFrosch and
agluszak
as code owners
github-actions
bot
added
the
awaiting-review
| } | ||
| } | ||
|
|
||
| private static String withForwardSlashes(Path p) { |
There was a problem hiding this comment.
com.intellij.openapi.util.io.FileUtilRt#toSystemIndependentName
There was a problem hiding this comment.
It turns out not so easy to refactor. This package is pretty low in the dependency hierarchy, and it doesn't depend on the IntelliJ API.
| @@ -57,17 +56,22 @@ public static Label of(String label) { | |||
| } | |||
|
|
|||
| public static Label fromWorkspacePackageAndName(String workspace, Path packagePath, Path name) { | |||
There was a problem hiding this comment.
add test too, to see that it works with WindowsPath?
There was a problem hiding this comment.
You mean the subclass of java.nio.file.Path? Unfortunately, it only exists on Windows JVMs. However, I have verified manually that my changes fix the issue on Windows.
| @@ -202,8 +202,8 @@ public int index(String s) { | |||
| public Query.StoredLabel indexLabel(Label l) { | |||
| return Query.StoredLabel.newBuilder() | |||
| .setWorkspace(index(l.getWorkspaceName())) | |||
| .setBuildPackage(index(l.getPackage().toString())) | |||
| .setName(index(l.getName().toString())) | |||
| .setBuildPackage(index(l.buildPackage())) | |||
There was a problem hiding this comment.
I searched for getPackage() and subsequent call to toString that introduces backslashes and found
There was a problem hiding this comment.
I'm not sure this has any adverse effects (FWIW TargetTreeTest passes on Windows), but I will fix this just in case.
odisseus
force-pushed
the
issue-7258
branch
2 times, most recently
from
4c01421 to
9bc1431
Compare
…o avoid occurrence of backslashes on Windows
Checklist
Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.
Discussion thread for this change
Issue number: #7258
Description of this change