-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Description
Edit by rsc, Dec 1 2020: Please note that the "likely accepted" answer here is #42328 (comment), specifically:
-
patterns passed to //go:embed are evaluated exactly with filepath.Glob, not "Glob with special cases".
So matching * keeps matching .foo, because it does in Glob. -
when you name a directory explicitly, as in //go:embed dir, the code that walks that directory collecting
all the files to embed will ignore names beginning with dot or underscore, the same as the go command does
for deciding what to build.
This means that //go:embed static/*
will embed .DS_Store if you really want to, while //go:embed static
will not.
This is forked off from #41191 to talk about a specific issue with the design as-accepted. #42325 and #42321 talk about the same problem, but they are very solution-focused and I think it is more appropriate to discuss the actual problem first.
Namely: @carlmjohnson has pointed out that the //go:embed
directive includes .DS_Store
files under MacOS. This is certainly working-as-designed, but I think it should be discussed whether that's actually the semantic we want. I want to talk about the .DS_Store
example specifically, but there are other dot-files with similar properties. .DS_Store
does illustrate something significant though, because it is a directory that is (AIUI, I'm not a Mac-user myself) created non-interactively by a third-party software in every subdirectory. So it is not explicitly created by the user and it is permanent (and will be re-created at some point, if deleted).
There have been several suggestions made so far, which IMO are deficient in one way or another:
- Manually clean up before
go build
or live with inclusion of any such files. IMO this puts unreasonable expectations on users. No matter how much we feel that in an ideal world, this should be what happens - I just don't think it is what will happen, in practice. - Run
git clean
beforego build
. This is certainly useful in CI/CD, but during regular development, it would also delete progress made. It's certainly not something I'd want to run before everygo test
orgo build
. In CI/CD it is useful, but it will probably still be forgotten by many people - though it's also far less of a problem, because the chance of pollution is low. - Add ways to exclude specific files. IMO
.DS_Store
is something that ~always should be excluded and as others have pointed out, there are many other patterns of files that should be excluded and would have to be listed. In effect, this would mean that ~every//go:embed
directive would have to list a non-canonical, long list of exclusions, to cover any tools used by people. It also still suffers from the problem that people have to know about the problem in the first place. So IMO it would still end up with accidental inclusions of.DS_Store
and similar files. - Make
*
not match dot-files. This isn't really helpful either, as even then, if a directory is named (either directly as//go:embed assets
or indirectly via a glob), we would still recursively include all subdirectories, including.DS_Store
. - Not include dot-files at all, unless explicitly mentioned. It can be argued that this is a "dirty" approach, because dot-files aren't actually special and I would agree with that. OTOH, it's IMO a) the approach leading to the least accidents with the lowest impact in actual practice and b) when an accident happens, it can be debugged and fixed most easily (simple testing will show the dot-file to be missing and the docs can clearly say that they have to be mentioned explicitly).
There are probably other approaches that can be discussed. It would also be a valid answer to close this as WAI. But if we release go 1.16 with embedding, we get locked into the semantics we implemented, so IMO this should be closed one way or another before releasing it into the wild.
Discussion summary (as of 2020-11-21, 10:00 UTC)
This is my best attempt at a fair and unbiased discussion-summary. It is necessarily subjective, though, so apologies if I left something out or misrepresented someone.
The resulting proposal from the discussion which is currently marked as "likely accept" is to have globs match as it currently does, but to exclude .*
and _*
files (editors remark: "as the go command" probably implies testdata
as well) from recursive directory walks when a directory is given explicitly.
There are a couple of open questions:
- Will hidden files also be excluded if a directory is matched by a glob, or just if it is mentioned by name (comment by @mpx)? Consensus seems to be that it would apply to globbed directories as well - that is, "expand globs first, then recursively walk every directory given with hidden files/dirs skipped".
- Does "hidden" also apply to extended filesystem attributes? (comment by @andrius4669)? There was one comment in favor by @Merovius and one comment against by @inliquid.
- Should
_*
andtestdata
really be skipped (comment by @mpx)? The case for them comes down to consistency withgo build
and seems significantly weaker than for.*
.
There where some alternatives suggested:
- Add a
**
matching operator tofilepath.Glob
- don't leave out hidden files when walking directories (comment by @ianthehat). @rsc remarks that this might be unexpectedly unspecified. Also, the presence of**
would also make it easier to specifically match all hidden files, if they are desired (comment by @Merovius) so can be argued in favor of this proposal as well. - Provide a simple way to test for accidental file inclusion, leave semantics as-is (comment by @nightlyone). The main argument against that is that it requires knowledge that this should be tested (comment by @Merovius).
- Provide a wrapper-fs that filters out undesirable files (comment by @seankhliao). In addition to the same argument as against tests, even the embedding of undesired files might be harmful, not just their use (comment by @SamWhited)
- Remove support for recursive directory inclusion for now, leave globs as-is. A separate go-generate tool can create complete file-listings and we can experiment syntax/semantics using such a tool during the next cycle (comment by @ianthehat).
- Remove both recursive directory walking and globs, only allow explicit lists (comment by @mpx).
There where some counter arguments:
- It is inconsistent and confusing that hidden files are included when using
*
but not in directory walks (comment by @dcormier). The proponents response is that we agree, but it might still be better than the current alternative. We need to make a tradeoff (comment by @Merovius). @SamWhited posted examples of where they embedded dot-files, which might be useful to inform that tredoff. - We need to have an escape hatch for "everything exactly as on disk" (comment by @seankhliao). The best escape hatch available so far is to use go-generate to create a list (comment by @mvdan), there might be the need for a more convenient one, if this comes up often enough.