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

PathFragment: test Windows mixed separator #22343

Closed

Conversation

sluongng
Copy link
Contributor

On Windows, if a user were to specify a PathFragment with mixed
separator such as

bazel build --profile='C:/foo\\bar' //...

then the profile file will be written as bar and recorded in
BuildToolLogs event as so.

Provide a test that demonstrates the current behavior to discuss whether
this is "correct" or not.

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label May 13, 2024
@sluongng sluongng force-pushed the sluongng/win-profile-path branch from e4a2a36 to e74a1c0 Compare May 13, 2024 09:41
@tjgq
Copy link
Contributor

tjgq commented May 13, 2024

I'm pretty sure that treating both / and \ as separators on Windows is intentional (see the implementation of WindowsOsPathPolicy.normalize). The backslashes are also canonicalized to forward slashes (i.e., you always get forward slashes when converting a PathFragment back to a String).

In terms of test coverage: instead of adding to testBasenameWindows, I'd suggest a separate test case verifying that the entire path is correctly broken down into segments.

On Windows, if a user were to specify a PathFragment with mixed
separator such as

```
bazel build --profile='C:/foo\\bar' //...
```

then the profile file will be written as `bar` and recorded in
BuildToolLogs event as so.

Provide a test that demonstrates the current behavior to discuss whether
this is "correct" or not.
@sluongng sluongng force-pushed the sluongng/win-profile-path branch from e74a1c0 to c90c146 Compare May 13, 2024 12:23
@sluongng
Copy link
Contributor Author

In terms of test coverage: instead of adding to testBasenameWindows, I'd suggest a separate test case verifying that the entire path is correctly broken down into segments.

I added some more cases with mixed usage of separators to the segment tests.

@tjgq tjgq added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 13, 2024
@sgowroji sgowroji added the team-Local-Exec Issues and PRs for the Execution (Local) team label May 14, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants