-
Notifications
You must be signed in to change notification settings - Fork 18k
x/tools/go/packages: seems to do nothing when given non-Go files #29899
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
Comments
Hi @mvdan. Can I help? :) |
If you mean picking up the issue, go ahead. It's not assigned. |
@mvdan Yeah that's what I meant. Thanks! Cool. :) |
@mvdan HI! Correct me if I'm wrong, but according to your example this is the scenario that's not working:
Right? While this is working:
It seems to me that it can read non-go files, but not if they are together with other files? |
Please see the docker instructions in the original post; they contain a complete example, along with what I'd consider two possible expected outcomes. The current outcome definitely seems wrong to me. I'm not sure that this is a good first issue if you're looking to contribute to go/packages. I think the fix here would be to error just like |
Oh! Hang on. I misread the whole thing. So sorry. This is the error:
For some reason I thought it's expected. While list says it's not a go file. There is a discrepancy in behaviour between the two. Right. I thought that the fact that there is no output on the third try is the problem. |
Oh I think I might know what's going on. It seems like it interprets it as a package name and of course it can't find that instead of a file. |
Hah. I think I located the error. Going to submit a CL soon once I narrowed it down to a unit test. 👍 |
So the problems appears to be that stderr is not checked for errors from go execute. Go correctly says this in stderr:
But this is being ignored. I need a nice way of handling this. :) |
Unfortunately because of #26319 I can't really check stderr for go failing. :/ I have to replicate the Args checker in the Loader... 🤔 |
@mvdan Is this a satisfactory outcome?
Please ignore the debug data. :) |
Change https://golang.org/cl/164663 mentions this issue: |
Found out why the error isn't working. :D if len(patterns) > 0 && strings.HasSuffix(patterns[0], ".go") {
return []*Package{GoFilesPackage(patterns)}
} It only checks the first value. Thus, if I change the multiple call to f2.bar f1.go, I'm seeing the proper error. Although proper is a strong word, because the errors will be the same, just for a different name. :D
Even though f1.go should have worked... |
Change https://golang.org/cl/166398 mentions this issue: |
This change modifies cmd/go/list to format the error correctly in case -e flag is set. It also fixes a bug where the package loader was only ever checking the first pattern if it had the go extension. This caused and error when a file without .go extension was not the first argument. Fixes #29899 Change-Id: I029bf4465ad4ad054434b8337c1d2a59369783da Reviewed-on: https://go-review.googlesource.com/c/go/+/166398 Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]>
Output:
The third case should either error, or give one package. Seems like there should be an error somewhere:
Not a huge problem, but it did throw me off. A tool I wrote on top of
go/packages
does absolutely nothing when given a Go file and some non-Go files after, which puzzled me./cc @matloob @ianthehat
The text was updated successfully, but these errors were encountered: