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.
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
go/tools: add gopackagesdriver #2858
go/tools: add gopackagesdriver #2858
Changes from 26 commits
a67afe0
bdd0556
76e6753
5a53ae2
d2b527a
ce61882
b08da87
5dd1e0e
168e90d
331470a
29d0b78
529b5ab
a0eeeec
79f0cb8
8f76121
1203bea
2657063
859a3b1
2190ab6
d69fad2
4719e91
07ad711
b3af1b1
079bb2a
070b4c3
69fef40
fb2df68
0d01dc3
c36a5eb
2773277
5923382
b962df0
98338cc
bcd9713
b81ac13
ae8788f
5b80ccd
34ee2be
dfe668a
81166a8
32aba61
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are there paths in particular that must be relative coming in? If so, let's document that here. If not, let's drop relative paths from
PATH
.We had a security issue in cmd/go with
PATH
on Windows, which implicitly checks the current directory, which could have some maliciousgcc.exe
in it. Russ described that in https://blog.golang.org/path-security.That may not really be relevant for this action, and it may not be possible since analysis might only have relative paths, but it's taught me to handle relative paths in
PATH
carefully.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 actually copied it from the stdlib builder
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.
Check for an error when closing writable files:
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 mean, yes, but is there a cleaner way to do this ?
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.
FYI, we'll eventually need an action that writes JSON files with more data (AST, type info). I believe gopls only requests the minimum from go/packages, then loads the rest on its own. Other clients will want a more complete response.
Definitely no need to do that now, but this might be a good spot for a TODO.
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.
Actually, the
GOPACKAGESDRIVER
should return this type as a result, which is pretty barebone: https://github.com/golang/tools/blob/master/go/packages/packages.go#L417-L433There 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.
What is 'x'?
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.
export file, an archive with only the
__.PKGDEF
file that contains export dataThere 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.
Let's rename to
export_file
, for clarity an consistency witharchive.data.export_file
.