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

embed: warn about dotfiles in embed.FS documentation #42321

Closed
earthboundkid opened this issue Nov 1, 2020 · 10 comments
Closed

embed: warn about dotfiles in embed.FS documentation #42321

earthboundkid opened this issue Nov 1, 2020 · 10 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@earthboundkid
Copy link
Contributor

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

Tip.

$ gotip version
go version devel +48be3ed Sat Oct 31 00:35:33 2020 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

Tried out the new embed.FS feature.

example.go:

package main

import (
	"embed"
	"fmt"
	"io/fs"
)

func main() {
	//go:embed example/*
	var files embed.FS
	des, _ := fs.ReadDir(files, "example")
	for _, de := range des {
		fmt.Printf("%q\n", de.Name())
	}
}

example:

total 1504
drwxr-xr-x@ 6 adhoc  staff   192 Oct 30 21:49 .
drwx------  8 adhoc  staff   256 Nov  1 13:34 ..
-rw-r--r--@ 1 adhoc  staff  6148 Oct 30 21:49 .DS_Store
-rw-r--r--  1 adhoc  staff     0 Oct 30 21:47 .dot
-rw-r--r--@ 1 adhoc  staff     0 Oct 30 21:48 Icon
-rw-r--r--  1 adhoc  staff     0 Oct 30 21:47 one.txt

What did you expect to see?

$ gotip run .
"one.txt"

What did you see instead?

$ gotip run .
".DS_Store"
".dot"
"Icon\r"
"one.txt"

There should be a warning in the documentation that dotfiles and hidden files are included.

@inliquid
Copy link

inliquid commented Nov 1, 2020

This is not a "documentation", but design issue, so this won't solve... anything?

@earthboundkid
Copy link
Contributor Author

I think documenting that dotfiles are included will solve 90% of this issue: it can be a surprising behavior. For the final 10%, I think there should be a way of excluding files, but I need to open another issue for that.

@zikaeroh
Copy link
Contributor

zikaeroh commented Nov 1, 2020

I think this speaks to a potential problem where the embedded data could differ between using someone else's module with embedded data (determined by whatever git/module serving rules dictate those contents) and checking code out locally to use it.

I'm also wondering if this means that a build can differ depending on whether or not GOPROXY was used, since now more than just the .go/.s/.c/.h files matter as they are laid out on disk.

@andig
Copy link
Contributor

andig commented Nov 1, 2020

I would actually like the ability of exluding specific files. even if we document dot file inclusion it would be good to have a way to ecxlude them, same as excluding something like assets.go would be nice.

@SamWhited
Copy link
Member

SamWhited commented Nov 1, 2020

I would actually like the ability of exluding specific files. even if we document dot file inclusion it would be good to have a way to ecxlude them, same as excluding something like assets.go would be nice.

I tentatively disagree. It seems to me that if you wanted to exclude certain files you shouldn't have selected those files in the first place using the glob.

I suspect allowing excluding individual files will end up with projects having giant lists of temp files to be excluded, one person excludes the .swp files that they accidentally included, then the next excludes the .DS_Store files their operating system included, then the next person excludes <your editor of choice's temp files>, etc. (rather like how we see projects with huge .gitignore files instead of people putting their specific editors in their user or system wide gitignore file). Instead I'd suggest it's better to not use a glob or configure your build to check for files you don't want.

@earthboundkid
Copy link
Contributor Author

This issue can be the proposal for documentation, and #42325 can be a proposal for excluding files.

@zikaeroh
Copy link
Contributor

zikaeroh commented Nov 1, 2020

I worry that attempting to split this up into many disparate specific proposals will cloud discussion trying to better understand implications and solutions to the various types of "unintended" files appearing in embedded filesystems.

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 2, 2020
@toothrot toothrot added this to the Backlog milestone Nov 2, 2020
@toothrot
Copy link
Contributor

toothrot commented Nov 2, 2020

/cc @rsc

@rsc
Copy link
Contributor

rsc commented Nov 4, 2020

We should certainly document for now that dot-files are included in a match of star.
We can use #42328 to discuss whether to keep that behavior.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 4, 2020
@rsc rsc self-assigned this Nov 4, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 4, 2020
@earthboundkid
Copy link
Contributor Author

earthboundkid commented Feb 17, 2021

Fixed in embed docs:

If a pattern names a directory, all files in the subtree rooted at that directory are embedded (recursively), except that files with names beginning with ‘.’ or ‘_’ are excluded.

The difference is that ‘image/*’ embeds ‘image/.tempfile’ while ‘image’ does not.

@golang golang locked and limited conversation to collaborators Feb 17, 2022
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants