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.

Sign up for GitHub

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

Jump to bottom

Caching of go list output breaks dependency resolution #82

Closed
lu4p opened this issue Aug 4, 2020 · 7 comments · Fixed by #88
Closed

Caching of go list output breaks dependency resolution #82

lu4p opened this issue Aug 4, 2020 · 7 comments · Fixed by #88

Comments

@lu4p
Copy link
Member

lu4p commented Aug 4, 2020 •
edited
Loading

Since https://github.com/mvdan/garble/pull/67 is merged the builds of ToRat, which use garble fail.

Build output: https://hub.docker.com/repository/registry-1.docker.io/lu4p/torat/builds/1f1fd73c-6cf5-4dc7-88d8-b89e26786589
@mvdan
Copy link
Member

mvdan commented Aug 4, 2020

Can you share steps to reproduce and a bit more detail?

@lu4p lu4p mentioned this issue Aug 4, 2020
Closed
@lu4p
Copy link
Member Author

lu4p commented Aug 4, 2020 •
edited
Loading

Sure to reproduce:

git clone https://github.com/lu4p/ToRat.git
cd ./ToRat 
sudo docker build . -t torat

@mvdan
Copy link
Member

mvdan commented Aug 4, 2020

Ah, the problem is likely the -tags flag, since we don't propagate that to go list. See the TODO in https://github.com/mvdan/garble/blob/c51e08ef37cbe5ac995c3c73a88c2a19b5ee2605/main.go#L299-L302.

I can probably take a look today or tomorrow. For now, use a slightly older version of garble.

mvdan added a commit that referenced this issue Aug 10, 2020
@mvdan
keep build flags when calling 'go list'
3c05a0d 
Otherwise any build flags like -tags won't be used, and we might easily
end up with errors or incorrect packages.

The common case with -tags is covered by one of the integration test
scripts. On top of that, we add a table-driven unit test to cover all
edge cases, since there are many we can do quickly in a unit test.

Fixes #82.
@mvdan mvdan mentioned this issue Aug 10, 2020
Merged
mvdan added a commit that referenced this issue Aug 10, 2020
@mvdan
keep build flags when calling 'go list'
5f02c45 
Otherwise any build flags like -tags won't be used, and we might easily
end up with errors or incorrect packages.

The common case with -tags is covered by one of the integration test
scripts. On top of that, we add a table-driven unit test to cover all
edge cases, since there are many we can do quickly in a unit test.

Fixes #82.
@alexmaloteaux alexmaloteaux mentioned this issue Aug 10, 2020
Open
@mvdan mvdan closed this as completed in #88 Aug 10, 2020
mvdan added a commit that referenced this issue Aug 10, 2020
@mvdan
keep build flags when calling 'go list'
d0e0147 
Otherwise any build flags like -tags won't be used, and we might easily
end up with errors or incorrect packages.

The common case with -tags is covered by one of the integration test
scripts. On top of that, we add a table-driven unit test to cover all
edge cases, since there are many we can do quickly in a unit test.

Fixes #82.
mvdan added a commit that referenced this issue Aug 10, 2020
@mvdan
add a regression test for #82
aa9767b 
The test case we had didn't have a realistic-looking module path with a
dot, so we hadn't noticed the bug with IndexByte.

Fix that. We verified that the new test fails if we undo the fix.
@lu4p lu4p reopened this Aug 10, 2020
@lu4p
Copy link
Member Author

lu4p commented Aug 10, 2020 

Step 15/19 : RUN cd ./cmd/client && garble -literals -seed=random build -tags "tor" -o /dist/client/client_linux && upx /dist/client/client_linux
 ---> Running in 15d60126df9e
go list error: exit status 1: can't load package: package /dist/client/client_linux: cannot find package

/dist/client/client_linux is somehow interpreted as a package

@zwass
Copy link
Contributor

zwass commented Aug 11, 2020 •
edited
Loading

I am running into the same issue with builds before d0e0147.

With builds after d0e0147, I get the following:

GOPRIVATE=<redacted> GOOS=darwin GOARCH=386 ./garble build -o main -tags foobar -ldflags -X <redacted>.clientID=foobar  <redacted>
go list error: exit status 1: go: finding module for package -X <redacted>.clientID=foobar
can't load package: package <redacted> is not in GOROOT (/usr/local/Cellar/go/1.14.4/libexec/src/<redacted>)
can't load package: package -tags is not in GOROOT (/usr/local/Cellar/go/1.14.4/libexec/src/-tags)
can't load package: package foobar is not in GOROOT (/usr/local/Cellar/go/1.14.4/libexec/src/foobar)
can't load package: package -ldflags is not in GOROOT (/usr/local/Cellar/go/1.14.4/libexec/src/-ldflags)
can't load package: package -X <redacted>.clientID=foobar : malformed module path "-X <redacted>.clientID=foobar ": leading dash

(Which seems to be the same issue @lu4p is having with the new changes?)

zwass added a commit to zwass/garble that referenced this issue Aug 11, 2020
@zwass
Revert "add a regression test for burrowers#82"
85d5fe2 
This reverts commit aa9767b.
@zwass
Copy link
Contributor

zwass commented Aug 11, 2020

For now I'm able to get the fix I needed for #89 by reverting the related commits in master: https://github.com/zwass/garble/tree/fixes

@mvdan
Copy link
Member

mvdan commented Aug 11, 2020

Sorry that this has turned into such a shitshow. I'll take a look tomorrow again.

@mvdan mvdan closed this as completed in 98113d0 Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

keep build flags when calling 'go list' burrowers/garble
3 participants
@zwass @mvdan @lu4p