Skip to content

Also retrieve file's package when fast-scanning for imports#209

Merged
jhump merged 5 commits intomainfrom
jh/add-package-to-fast-imports-scanner
Nov 29, 2023
Merged

Also retrieve file's package when fast-scanning for imports#209
jhump merged 5 commits intomainfrom
jh/add-package-to-fast-imports-scanner

Conversation

@jhump
Copy link
Member

@jhump jhump commented Nov 29, 2023

This augments the experimental fast-scanner imports.ScanForImports to also return the package for each scanned file.

@jhump jhump requested a review from bufdev November 29, 2023 00:18
Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might move this to fastscan.Scan or something, but all good for me!

@jhump
Copy link
Member Author

jhump commented Nov 29, 2023

I might move this to fastscan.Scan or something

Sounds like a good idea, so I went ahead and did this.

@jhump jhump enabled auto-merge (squash) November 29, 2023 14:39
@jhump jhump merged commit fa71488 into main Nov 29, 2023
@jhump jhump deleted the jh/add-package-to-fast-imports-scanner branch November 29, 2023 14:41
@bufdev
Copy link
Member

bufdev commented Dec 3, 2023

Something about this change broke the integration tests for the new workspaces - I think it is that some imports are no longer getting picked up:

https://github.com/bufbuild/buf/commits/bufmod

cd path/to/buf-checkout
git fetch origin
git checkout bufmod
git checkout a7de4fb19a7ea110e8ccecc992eaff4bfbb2d98c # right before i reverted protocompile to before this PR
go test ./private/buf/bufworkspace

--- FAIL: TestBasic (0.01s)
    dagtest.go:64:
        	Error Trace:	/Users/foo/dev/buf/private/pkg/dag/dagtest/dagtest.go:64
        	           				/Users/foo/dev/buf/private/buf/bufworkspace/workspace_test.go:76
        	Error:      	Not equal:
        	           	expected: []dagtest.ExpectedNode[string]{dagtest.ExpectedNode[string]{Key:"buf.testing/acme/bond", Outbound:[]string{"buf.testing/acme/date", "buf.testing/acme/geo", "buf.testing/acme/money"}}, dagtest.ExpectedNode[string]{Key:"buf.testing/acme/date", Outbound:[]string{"buf.testing/acme/extension"}}, dagtest.ExpectedNode[string]{Key:"buf.testing/acme/extension", Outbound:[]string{}}, dagtest.ExpectedNode[string]{Key:"buf.testing/acme/geo", Outbound:[]string{}}, dagtest.ExpectedNode[string]{Key:"buf.testing/acme/money", Outbound:[]string{}}, dagtest.ExpectedNode[string]{Key:"finance/portfolio/proto", Outbound:[]string{"buf.testing/acme/bond", "buf.testing/acme/extension"}}}
        	           	actual  : []dagtest.ExpectedNode[string]{dagtest.ExpectedNode[string]{Key:"buf.testing/acme/bond", Outbound:[]string{"buf.testing/acme/date"}}, dagtest.ExpectedNode[string]{Key:"buf.testing/acme/date", Outbound:[]string{"buf.testing/acme/extension"}}, dagtest.ExpectedNode[string]{Key:"buf.testing/acme/extension", Outbound:[]string{}}, dagtest.ExpectedNode[string]{Key:"finance/portfolio/proto", Outbound:[]string{"buf.testing/acme/bond"}}}
        	           	
        	           	Diff:
        	           	--- Expected
        	           	+++ Actual
        	           	@@ -1,8 +1,6 @@
        	           	-([]dagtest.ExpectedNode[string]) (len=6) {
        	           	+([]dagtest.ExpectedNode[string]) (len=4) {
        	           	 (dagtest.ExpectedNode[string]) {
        	           	  Key: (string) (len=21) "buf.testing/acme/bond",
        	           	-  Outbound: ([]string) (len=3) {
        	           	-   (string) (len=21) "buf.testing/acme/date",
        	           	-   (string) (len=20) "buf.testing/acme/geo",
        	           	-   (string) (len=22) "buf.testing/acme/money"
        	           	+  Outbound: ([]string) (len=1) {
        	           	+   (string) (len=21) "buf.testing/acme/date"
        	           	  }
        	           	@@ -21,16 +19,5 @@
        	           	 (dagtest.ExpectedNode[string]) {
        	           	-  Key: (string) (len=20) "buf.testing/acme/geo",
        	           	-  Outbound: ([]string) {
        	           	-  }
        	           	- },
        	           	- (dagtest.ExpectedNode[string]) {
        	           	-  Key: (string) (len=22) "buf.testing/acme/money",
        	           	-  Outbound: ([]string) {
        	           	-  }
        	           	- },
        	           	- (dagtest.ExpectedNode[string]) {
        	           	  Key: (string) (len=23) "finance/portfolio/proto",
        	           	-  Outbound: ([]string) (len=2) {
        	           	-   (string) (len=21) "buf.testing/acme/bond",
        	           	-   (string) (len=26) "buf.testing/acme/extension"
        	           	+  Outbound: ([]string) (len=1) {
        	           	+   (string) (len=21) "buf.testing/acme/bond"
        	           	  }
        	Test:       	TestBasic
FAIL
FAIL	github.com/bufbuild/buf/private/buf/bufworkspace	0.243s
FAIL

Thats all the info I have right now, but here is where it is called: https://github.com/bufbuild/buf/blob/a7de4fb19a7ea110e8ccecc992eaff4bfbb2d98c/private/bufpkg/bufmodule/module_set.go#L472 and here are the test files: https://github.com/bufbuild/buf/tree/a7de4fb19a7ea110e8ccecc992eaff4bfbb2d98c/private/buf/bufworkspace/testdata/basic https://github.com/bufbuild/buf/blob/a7de4fb19a7ea110e8ccecc992eaff4bfbb2d98c/private/buf/bufworkspace/workspace_test.go

@jhump
Copy link
Member Author

jhump commented Dec 4, 2023

To close the loop on the issue brought up above: this turned out to be an error in the test source code that was being used. The change in this PR that tickled it was that this change made the parsing of imports a little bit more precise, which means it could fail in the face of certain kinds of syntax errors in the source. Fixing the syntax errors in the test source resolved the issue.

See #214 for more updates to this package. When these kinds of syntax errors are identified, they will be returned to the caller. That PR also adds some tests, which caught an issue in this package with handling of public and weak imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants