unused_deps: Support BUILD.bazel files #728

aljoscha · 2019-10-05T10:23:21Z

Before, we would only support BUILD files. Now, we check if the BUILD
file exists, if not we add the .bazel extension and return that.

I didn't add a test because there previously didn't exist a test for InterpretLabelForWorkspaceLocation and it's a bit more involved because it includes parsing/finding the WORKSPACE files, among other things. If you have a suggestion for a possible test I'm happy to take a stab at it.

In my local testing (with Bazel 0.29.1) build_deps now successfully finds unused dependencies and prints buildozer commands for removing them.

This fixes #204
This fixes #441

I have a Google ICLA on file.

Before, we would only support BUILD files. Now, we check if the BUILD file exists, if not we add the .bazel extension and return that. This fixes bazelbuild#204 This fixes bazelbuild#441

aljoscha · 2019-10-05T11:54:06Z

I added a test case but I couldn't figure out what the second part of the tested function is meant to do. I don't know if any production code or test code ever has the case that the prefix of target is not //. Anyone know what's up with this?

This was discovered by the newly added test for InterpretLabelForWorkspaceLocation.

aljoscha · 2019-10-05T12:03:56Z

I added another commit that fixes path handling on Windows. This was a pre-existing problem but was only discovered by the newly added test.

vladmos · 2019-10-07T12:38:30Z

edit/edit.go

@@ -59,7 +58,7 @@ func ParseLabel(target string) (string, string, string) {
 	if len(parts) == 1 {
 		if strings.HasPrefix(target, "//") || tables.StripLabelLeadingSlashes {
 			// "//absolute/pkg" -> "absolute/pkg", "pkg"
-			return repo, parts[0], path.Base(parts[0])
+			return repo, parts[0], filepath.Base(parts[0])


Why filepath? Labels always have forward slashes regardless of current operating system.

Ah, you're right. It seems I went a bit overboard when I fixed file path handling for windows.

Reverting 👌

vladmos · 2019-10-07T13:54:17Z

edit/edit.go

 	} else {
 		buildFile = "BUILD"
+		if !isFile(buildFile) {


Instead of duplicating the check, you can move it after the if-else block.

pushed the change for that

vladmos · 2019-10-07T13:56:45Z

edit/edit_test.go

+	if err := os.MkdirAll(filepath.Join(tmp, "a", "b"), 0755); err != nil {
+		t.Fatal(err)
+	}
+	// todo(aljoscha): See if we should hardcode "WORKSPACE" here or should use wspace.workspaceFile


You have two pending TODOs, are you going to keep them? If so, it's better to file corresponding bugs and use links for them instead of your username, this way it'll be much easier to track them.

Ah yes, sorry about that. I meant them mostly as questions for the reviewer or someone familiar with the code

So for the first one: should I leave "WORKSPACE" here and assume that this will always be the filename of workspace files? Or I could change wspace.workspaceFile to WorkspaceFile and use that in my new test.

For the second one: I don't know when the second part of the InterpretLabelForWorkspaceLocation function would be used. If I remove the second part of the method, that is those lines:

if isFile(pkg) { // allow operation on other files like WORKSPACE buildFile = pkg pkg = filepath.Join(relativePath, filepath.Dir(pkg)) return } if pkg != "" { buildFile = pkg + "/BUILD" if !isFile(buildFile) { // try it with the .bazel extension buildFile += ".bazel" } } else { buildFile = "BUILD" if !isFile(buildFile) { // try it with the .bazel extension buildFile += ".bazel" } } pkg = filepath.Join(relativePath, pkg)

bazel test /... still passes. Which doesn't mean that we don't need them, it might just be missing test coverage.

I think it's ok to hardcode the "WORKSPACE" string for now.

The second part of the function will be executed if the following check doesn't pass: strings.HasPrefix(target, "//"), i.e. the label is not absolute but relative, e.g. ":a" or ":a/b".

Thanks! And I'm sorry, this is taking up quite a bit of your time.

I understand the flow of the function, I wasn't sure about the behaviour because it was not previously covered by tests. And the existing behaviour is a bit weird (or doesn't throw errors). For example:

/path/to/WORKSPACE:foo gives ("/path/to/WORKSPACE", "/path/to", "foo")

/path/to/WORKSPACE gives ("BUILD", "", "/path/to/WORKSPACE")
(the tuple is the result of InterpretLabelForWorkspaceLocation, which is (buildFile, pkg, rule)

The first one seems (is?) correct, the second result seems to be bogus, but this is what the existing code spits out.

I think it worth investigating, but right now it's safer to leave it as it is.

vladmos · 2019-10-09T17:30:56Z

edit/edit.go

 		return
 	}
 	if pkg != "" {
-		buildFile = pkg + "/BUILD"
+		buildFile = filepath.Join(pkg, "/BUILD")


Good catch, but now the tests fail on Windows, you probably need to update them by replacing a slash with filepath.Separator.

Also, shouldn't it be "BUILD" instead of "/BUILD"?

I only need to remove the / because Join 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

{"/abs/path/to/WORKSPACE:rule", "", "/abs/path/to/WORKSPACE", "rule"},

. The reason is that the code simply splits on the :, which is a part of the path on windows.

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.

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.

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.

This reverts commit 023552c.

vladmos · 2019-10-09T20:50:10Z

Thanks!

I can update and squash with a single click if there are no merging conflicts, so that’s not necessary.

unused_deps: Support BUILD.bazel files

76e22dd

Before, we would only support BUILD files. Now, we check if the BUILD file exists, if not we add the .bazel extension and return that. This fixes bazelbuild#204 This fixes bazelbuild#441

aljoscha requested review from laurentlb, pmbethe09 and vladmos as code owners October 5, 2019 10:23

googlebot added the cla: yes label Oct 5, 2019

fixup! add test case

7f4fad7

Fix filepath handling on Windows in edit.go

df97615

This was discovered by the newly added test for InterpretLabelForWorkspaceLocation.

vladmos reviewed Oct 7, 2019

View reviewed changes

fixup! Fix filepath handling on Windows in edit.go

be784a0

aljoscha requested a review from vladmos October 7, 2019 13:46

vladmos requested changes Oct 7, 2019

View reviewed changes

fixup! unused_deps: Support BUILD.bazel files

048ec01

aljoscha requested a review from vladmos October 7, 2019 15:22

aljoscha added 3 commits October 9, 2019 15:21

fixup! remove TODO from test

edca01e

fixup! add test case for paths without //

96b69a8

fixup! fix test failure on windows

5eb33b6

vladmos reviewed Oct 9, 2019

View reviewed changes

aljoscha added 3 commits October 9, 2019 19:33

fixup! fix test failure on windows

023552c

Revert "fixup! fix test failure on windows"

6aceebb

This reverts commit 023552c.

fixup! remove test cases that are failing on windows

1eea6a3

vladmos self-requested a review October 9, 2019 20:50

vladmos approved these changes Oct 9, 2019

View reviewed changes

Merge branch 'master' into support-dot-bazel-files

ec2f38c

vladmos merged commit a795ddb into bazelbuild:master Oct 9, 2019

aljoscha deleted the support-dot-bazel-files branch October 10, 2019 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unused_deps: Support BUILD.bazel files #728

unused_deps: Support BUILD.bazel files #728

aljoscha commented Oct 5, 2019

aljoscha commented Oct 5, 2019

aljoscha commented Oct 5, 2019

vladmos Oct 7, 2019

aljoscha Oct 7, 2019

vladmos Oct 7, 2019

aljoscha Oct 7, 2019

vladmos Oct 7, 2019

aljoscha Oct 7, 2019 •

edited

Loading

vladmos Oct 9, 2019

aljoscha Oct 9, 2019

vladmos Oct 9, 2019

vladmos Oct 9, 2019

vladmos Oct 9, 2019

aljoscha Oct 9, 2019

vladmos Oct 9, 2019

aljoscha Oct 9, 2019

aljoscha Oct 9, 2019

vladmos commented Oct 9, 2019

unused_deps: Support BUILD.bazel files #728

unused_deps: Support BUILD.bazel files #728

Conversation

aljoscha commented Oct 5, 2019

aljoscha commented Oct 5, 2019

aljoscha commented Oct 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljoscha Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladmos commented Oct 9, 2019

aljoscha Oct 7, 2019 •

edited

Loading