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

go/packages: Package.OtherFiles contains globs #69555

Closed
josharian opened this issue Sep 20, 2024 · 5 comments
Closed

go/packages: Package.OtherFiles contains globs #69555

josharian opened this issue Sep 20, 2024 · 5 comments
Labels
WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Unreleased

Comments

@josharian
Copy link
Contributor

Go version

go version go1.23.1

Output of go env in your module/workspace:

-

What did you do?

Used go/packages on a package containing this code:

//go:embed *
var FS embed.FS

Then did:

	packages.Visit(pkgs, nil, func(p *packages.Package) {
  	for _, f := range p.OtherFiles {
   		fmt.Println(f)

What did you see happen?

Got output like:

/path/to/package/*

That * comes from the go:embed directive, I believe.

What did you expect to see?

The glob being fully resolved. The docs for OtherFiles say:

	// OtherFiles lists the absolute file paths of the package's non-Go source files,
	// including assembly, C, C++, Fortran, Objective-C, SWIG, and so on.
	OtherFiles []string

* is not an absolute file path in this case. :)
@josharian
Copy link
Contributor Author

josharian commented Sep 20, 2024 •
edited
Loading

Worse than that, if the embed directive is something like:

//go:embed *.txt *.md
var FS embed.FS

then OtherFiles includes the "path":

/path/to/package/*.txt *.md

(exactly like that, including the space)

@adonovan
Copy link
Member

Hi @josharian, I can't reproduce it at tip using the code below (a patch to gopackages).

xtools$ git diff
diff --git a/go/packages/gopackages/main.go b/go/packages/gopackages/main.go
index 9a0e7ad92..9d7994362 100644
--- a/go/packages/gopackages/main.go
+++ b/go/packages/gopackages/main.go
@@ -114,7 +114,7 @@ func (app *application) Run(ctx context.Context, args ...string) error {
        default:
                return tool.CommandLineErrorf("invalid mode: %s", app.Mode)
        }
-       cfg.Mode |= packages.NeedModule
+       cfg.Mode |= packages.NeedModule | packages.NeedEmbedPatterns
 
        lpkgs, err := packages.Load(cfg, args...)
        if err != nil {
@@ -178,6 +178,8 @@ func (app *application) print(lpkg *packages.Package) {
        }
        fmt.Printf("\tpackage %s\n", lpkg.Name)
 
+       log.Println(lpkg, lpkg.OtherFiles, lpkg.EmbedPatterns)
+
        // characterize type info
        if lpkg.Types == nil {
                fmt.Printf("\thas no exported type info\n")

xtools$ go run ./go/packages/gopackages ./gopls/internal/server   # the server package contains an embed glob
Go package "golang.org/x/tools/gopls/internal/server":
	module golang.org/x/tools/gopls@
	package server
2024/09/20 14:04:58 golang.org/x/tools/gopls/internal/server [] [/Users/adonovan/w/xtools/gopls/internal/server/assets/*]
	has no exported type info
...

Observe that the log statement shows no rogue OtherFiles, but EmbedPatterns contains globs as it should.

Could you provide a repro in a similar form? Thanks.

@josharian
Copy link
Contributor Author

Hmm. Trying to bisect between my code and yours...

In the meantime, FYI, LoadMode.String is missing a few of the newer Need* constants.

@cagedmantis cagedmantis added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 20, 2024
@cagedmantis cagedmantis added this to the Unreleased milestone Sep 20, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614875 mentions this issue: go/packages: fix LoadMode.String

@josharian
Copy link
Contributor Author

OK, found the problem. This is in go-fuzz, in code so old NeedEmbed didn't exist...so it was manually working around it. :( So this was entirely user error. Sorry about the noise.

gopherbot pushed a commit to golang/tools that referenced this issue Sep 25, 2024
@adonovan
go/packages: fix LoadMode.String
Loading
Loading status checks…
eb774f6 
It had neglected some newer entries.
Also, eliminate unnecessary tokens in the string.

Also, document the Load* mode bit sets.
They may be deprecated but they are useful, convenient,
and widely used. Sometime remind me why they are deprecated.

Updates golang/go#69555

Change-Id: Id0316cc1e9d39adf64e20db5194b867c3d19ab5d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614875
Auto-Submit: Alan Donovan <adonovan@google.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@josharian @cagedmantis @adonovan @gopherbot