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.

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

cmd/go: improve diagnostic and documentation for invalid //go:embed file name #44486

Open
catundercar opened this issue Feb 22, 2021 · 8 comments
Open
Labels
NeedsFix
Milestone

Comments

@catundercar
Copy link
Contributor

@catundercar catundercar commented Feb 22, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="off"
GOARCH="amd64"
GOBIN="/root/go/bin"
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/go"
GOPRIVATE=""
GOPROXY="https://goproxy.io"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build162353995=/tmp/go-build -gno-record-gcc-switches"

What did you do?

my directory test tree is:

├── 00|00.test
└── test

0 directories, 2 files

but the fs.ReadDir() only returns single file: test.
the code is:

//go:embed test
var efs embed.FS

func main() {
    efps, _ := efs.ReadDir("test")
    for _, efp := range efps {
        fmt.Println(efp.Name())
    } 
}

What did you expect to see?

like os.ReadDir(), want two files return(return all files).

What did you see instead?

Just return single file: test.

@catundercar
Copy link
Contributor Author

@catundercar catundercar commented Feb 22, 2021

it seems like the | can not pass the function isBadEmbedName

@catundercar
Copy link
Contributor Author

@catundercar catundercar commented Feb 22, 2021

I edit the source code of go in src/cmd/go/internal/load/pkg.go.I comment the logic module.CheckFilePath() in isBadEmbedName function line 2095-2097 and rebuild a go binary, then use the new binary build my code, and it works~
But I think it's not appropriatly.

@catundercar
Copy link
Contributor Author

@catundercar catundercar commented Feb 22, 2021

@rsc Are there any way to support some special symbol by appropriate way?

@ALTree ALTree added the NeedsInvestigation label Feb 22, 2021
@ALTree ALTree added this to the Go1.17 milestone Feb 22, 2021
@beoran
Copy link

@beoran beoran commented Feb 22, 2021

@Mrliu8023

Is there any reason that the name of the directory has to be 00|00.test? To me, a name like, e.g. 00_00.test seems to be just as useful and less obscure.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 22, 2021

The go command correctly reports an error for this file, but we should improve the error message and the embed documentation to explain why.

Currently the documentation says:

Patterns must not match files outside the package's module, such as ‘.git/*’ or symbolic links. Matches for empty directories are ignored. After that, each pattern in a //go:embed line must match at least one file or non-empty directory.

A file named 00|00.test cannot be included in a module because it contains characters that are meaningful to the shell. That's what module.CheckFilePath is checking. Its documentation explains the restrictions.

CheckFilePath checks that a slash-separated file path is valid. The definition of a valid file path is the same as the definition of a valid import path except that the set of allowed characters is larger: all Unicode letters, ASCII digits, the ASCII space character (U+0020), and the ASCII punctuation characters “!#$%&()+,-.=@[]^_{}~”. (The excluded punctuation characters, " * < > ? ` ' | / \ and :, have special meanings in certain shells or operating systems.)

//go:embed can't match files that aren't part of a module because packages would include different sets of files depending on whether they were built as part of the main module or a dependency.

@jayconrod jayconrod added NeedsFix and removed NeedsInvestigation labels Feb 22, 2021
@jayconrod jayconrod removed this from the Go1.17 milestone Feb 22, 2021
@jayconrod jayconrod added this to the backlog milestone Feb 22, 2021
@jayconrod jayconrod removed this from the backlog milestone Feb 22, 2021
@jayconrod jayconrod added this to the Backlog milestone Feb 22, 2021
@jayconrod jayconrod changed the title embed: not support | or full-width symbol file name cmd/go: improve diagnostic and documentation for invalid //go:embed file name Feb 22, 2021
@catundercar
Copy link
Contributor Author

@catundercar catundercar commented Feb 22, 2021

@Mrliu8023

Is there any reason that the name of the directory has to be 00|00.test? To me, a name like, e.g. 00_00.test seems to be just as useful and less obscure.

Because i can't control the input of users. Might I need give more information in my document.

@catundercar
Copy link
Contributor Author

@catundercar catundercar commented Feb 22, 2021

The go command correctly reports an error for this file, but we should improve the error message and the embed documentation to explain why.

Currently the documentation says:

Patterns must not match files outside the package's module, such as ‘.git/*’ or symbolic links. Matches for empty directories are ignored. After that, each pattern in a //go:embed line must match at least one file or non-empty directory.

A file named 00|00.test cannot be included in a module because it contains characters that are meaningful to the shell. That's what module.CheckFilePath is checking. Its documentation explains the restrictions.

CheckFilePath checks that a slash-separated file path is valid. The definition of a valid file path is the same as the definition of a valid import path except that the set of allowed characters is larger: all Unicode letters, ASCII digits, the ASCII space character (U+0020), and the ASCII punctuation characters “!#$%&()+,-.=@[]^_{}~”. (The excluded punctuation characters, " * < > ? ` ' | / \ and :, have special meanings in certain shells or operating systems.)

//go:embed can't match files that aren't part of a module because packages would include different sets of files depending on whether they were built as part of the main module or a dependency.

Yes.The embed files should have the same rule with .go files.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 21, 2022

Change https://go.dev/cl/413494 mentions this issue: embed: additional embed file name check rules #44486

@dmitshur dmitshur removed this from the Backlog milestone Jun 23, 2022
@dmitshur dmitshur added this to the Go1.19 milestone Jun 23, 2022
@dmitshur dmitshur removed this from the Go1.19 milestone Jun 23, 2022
@dmitshur dmitshur added this to the Backlog milestone Jun 23, 2022
gopherbot pushed a commit that referenced this issue Jun 23, 2022
For #44486

Change-Id: I66af9f7a9f95489a41fd6710e50bdd7878f78b85
Reviewed-on: https://go-review.googlesource.com/c/go/+/413494
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

6 participants