proposal: embed: allow embeding files in a parent directory

[gucio321]

INTRO

Since GO 1.16 was released, many people feel really confused due to the fact that they are unable to use newly added go:embed pattern, to include files in a parent directory.

CAUSE

As stated in #46056 (comment) the issue happens, because embed.FS implements io/fs.FS. Documentation says:

	// Open opens the named file.
	//
	// When Open returns an error, it should be of type *PathError
	// with the Op field set to "open", the Path field set to name,
	// and the Err field describing the problem.
	//
	// Open should reject attempts to open names that do not satisfy
	// ValidPath(name), returning a *PathError with Err set to
	// ErrInvalid or ErrNotExist.

so in theory .. patterns does not satisfy fs.ValidPath

Path names must not contain an element that is “.” or “..” or the empty string, except for the special case that the root directory is named “.”.

WHAT IS WRONG WITH THIS APPROACH

Although I don't understand why ValidPath does not support dot-dot and I wouldn't challange this, I am sure that it isn't a right approach in case of go:embed (embed.FS). - There is no real reason for disallowing .. in this particular case.
Here is the exact chapter of design doc: https://go.googlesource.com/proposal/+/master/design/draft-embed.md?pli=1#dot_dot_module-boundaries_and-file-name-restrictions
it states that .. pattern should be allowed as long as it doesn't cross module boundries (for the obvious reasons).
Generally, I see no reason why it shouldn't be done this way

Usa cases

• embeding a README.md in a go file
• making assets folder in the root of project

Alternative

In my opinion the following alternative could be considered:
In order to allow embeding files in a directory being higher in tree, a special "path-prefix" / could be introduced.
For example //go:embed /README.md, where / means go.mod's directory.

Adventages

• does not break compatibility with past versions
• fix the issue

disadventages

May be a bit confusing for some linux users (where / means filesystem's root 😄).

REFERENCE

https://go.googlesource.com/proposal/+/master/design/draft-embed.md
#46056

[Jorropo]

I'm a bit confused, your proposal is that //go:embed .., //go:embed ../../florb, ... would load some ressource that might be in parent directories as long as this does not cross the module boundary ?

[gucio321]

I'm a bit confused, your proposal is that //go:embed .., //go:embed ../../florb, ... would load some ressource that might be in parent directories as long as this does not cross the module boundary ?

exactly

EDIT ofc .. is relative path for file containing go:embed directive.

[seankhliao]

embed.FS is documented as implementing io/fs.FS
as part of compliance with said interface, .. is explicitly disallowed and tested for in testing/fstest https://cs.opensource.google/go/go/+/refs/tags/go1.20:src/testing/fstest/testfs.go;l=155

The section you quoted from the draft design says it should be disallowed entirely.

The solution to this would be to make the package at the root level export the embedded FS. Import cycles don't have to be an issue if you organize your code structure appropriately, or just pass it down as an argument to dependencies rather than importing it.

[gucio321]

embed.FS is documented as implementing io/fs.FS as part of compliance with said interface, .. is explicitly disallowed and tested for in testing/fstest https://cs.opensource.google/go/go/+/refs/tags/go1.20:src/testing/fstest/testfs.go;l=155

The section you quoted from the draft design says it should be disallowed entirely.

@seankhliao and? Despite this fact, I can't see any reason for closing this proposal (fact that "It is tested for in testing/fast does not mean that it is not possible to be changed, does it?). Over that, there is also an alternative solution (in Alternative section).
Could you please re-open this in order to carry out a standard Proposal Process. According to point 2:
A discussion on the issue tracker aims to triage the proposal into one of three outcomes:

2. A discussion on the issue tracker aims to triage the proposal into one of three outcomes:

Thank you!

[seankhliao]

proposals need to be achievable within the guarantees we make as part of https://go.dev/doc/go1compat
changing embed.FS to support .. isn't compatible with that

[Jorropo]

@seankhliao I think this is a valid proposal, at least for the //go:embed directive, all already compiling programs will continue to compile and have the same behaviour. Previously invalid programs (using //go:embed ../example/path where [\.\./]+ is the module root or using //go:embed /example/path) will now be valid programs.

AFAIK this is not breaking the go1compat promise.

I think the embed.FS should be removed tho, we don't want to start having to bundle complete modules just in case some runtime input start doing .., ...

[ianlancetaylor]

@Jorropo We promise today that embed.FS implements fs.FS. Changing that will break existing programs that pass a variable of type embed.FS to a function that expects an argument of type fs.FS. We can't make such a change.

[gucio321]

So lets make it language-change if really no way around

[ianlancetaylor]

@gucio321 I understand that you find this to be frustrating, but we are not going to change the language merely so that people can embed files in a parent directory.

[gucio321]

ok, @ianlancetaylor I see your point, but I have one more question:

@Jorropo We promise today that embed.FS implements fs.FS. Changing that will break existing programs that pass a variable of type embed.FS to a function that expects an argument of type fs.FS. We can't make such a change.

ok, so I see whay .. pattern is not possible now.
But what about doing something like this:

• if go:embed is of form /example/path it actually "returns" an fs.FS starting at module root so that this / is trimmed. This way the following code should work:

//go:embed /example/path
var data embed.FS

// ...
f, _ := data.Open("example/path/file.txt")

and will not break compatibility, will it?

• embed.FS itself will not be affected anyway, just implementation of go:embed directive's implementation wil be modified.

[ianlancetaylor]

I may have missed something but I agree that doesn't seem to break compatibility. I think it might be confusing for people reading the code, but otherwise I think it could work. Want to open a new proposal so that people can skip the discussion on this one?

[Jorropo]

@ianlancetaylor I was saying that //go:embed .. does not break the compatibility promise.

.
├── assets
│   └── dog.png
├── go.mod
└── serve
    └── main.go

I think that this code inside main.go:

//go:embed ../asserts
var assets embed.FS

// ...
dog, err := assets.Open("dog.png")

or

//go:embed /asserts
var assets embed.FS

// ...
dog, err := assets.Open("dog.png")

would work without breaking anything. In other words, I don't understand why this is not a valid proposal.

I can think of counter arguments:

• do we really want to privilege the directory of go.mod ?
  This would make it harder to refactor code that use .. embeds because previous embeds may now not work, it can also be surprising that some .go file 3 level deep can reference your module root.

[gucio321]

@Jorropo

//go:embed ../asserts
var assets embed.FS

// ...
dog, err := assets.Open("dog.png")

it will break, since this embed.FS actually looks this way

> assets
   > dog.png

so you would need assets.Open("../assets/dog.png") I think

[Jorropo]

ah ok, I see thx

[atljoseph]

For the case of a VERSION file, we were able to get past this by using symbolic link from VERSION at repo root (go mod root) to VERSION file in embeds package. The embeds package loads the real VERSION file, but anytime a dev opens and edits VERSION, it really edits the one in the embeds directory. Helps a lot with keeping the traditional VERSION file visible at repo root.