-
Notifications
You must be signed in to change notification settings - Fork 417
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
unused_deps: Support BUILD.bazel files #728
Merged
Merged
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
76e22dd
unused_deps: Support BUILD.bazel files
aljoscha 7f4fad7
fixup! add test case
aljoscha df97615
Fix filepath handling on Windows in edit.go
aljoscha be784a0
fixup! Fix filepath handling on Windows in edit.go
aljoscha 048ec01
fixup! unused_deps: Support BUILD.bazel files
aljoscha edca01e
fixup! remove TODO from test
aljoscha 96b69a8
fixup! add test case for paths without //
aljoscha 5eb33b6
fixup! fix test failure on windows
aljoscha 023552c
fixup! fix test failure on windows
aljoscha 6aceebb
Revert "fixup! fix test failure on windows"
aljoscha 1eea6a3
fixup! remove test cases that are failing on windows
aljoscha ec2f38c
Merge branch 'master' into support-dot-bazel-files
vladmos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Good catch, but now the tests fail on Windows, you probably need to update them by replacing a slash with
filepath.Separator
.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.
Also, shouldn't it be
"BUILD"
instead of"/BUILD"
?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 only need to remove the
/
becauseJoin
already adds that. (I don't have windows so I only notice it from the buildkite run. Getting this to pass on windows is a bit tricky, I'll notify you again once I have it figured out.I think the gist of it is that
ParseLabel
does not work for absolute windows paths. It was only tested on unix-style paths:buildtools/edit/edit_test.go
Line 46 in 96b69a8
:
, which is a part of the path on windows.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.
That's probably the reason, at least it can explain the following error: https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/bf49dbeb-eb10-4ee6-bbdc-669c127fbc3f/edit%5Cgo_default_test%5Cattempt_1.log
Feel free to just revert the change to this line if you don't want to fix it now, it's not directly relevant to your pull request.
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.
Another suggestion is to ignore this issue for now, remove the last two test cases, and create a new issue for fixing the parsing issues on windows.
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 removed the unrelated change and also removed the test cases (that I added) which are failing on windows. I can add a new ticket for these issues afterwards if you'd like that.
Also: please let me know if I should rebase/squash this once you're done with the review. I'm not familiar with how you handle that on this project.