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: support overlays in the go command #39958

Closed
stamblerre opened this issue Jun 30, 2020 · 27 comments
Closed

cmd/go: support overlays in the go command #39958

stamblerre opened this issue Jun 30, 2020 · 27 comments
Labels
FeatureRequest FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@stamblerre
Copy link
Contributor

This has been discussed fairly often, but I figured it was worth filing an issue for tracking purposes.

Many tools, gopls in particular, work on unsaved files. As a result, tools like go/packages support overlays, which can replace file contents or represent entirely new files. The overlay logic for go list in go/packages is complex and error-prone, as it effectively needs to recreate the behavior of go list. It is a constant source of gopls bugs, many of which (#39646 for a recent example) manifest in seemingly unrelated ways. Supporting overlays natively in the go command would eliminate all of this complicated logic and this source of errors.

@dmitshur dmitshur added FeatureRequest GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 1, 2020
@dmitshur dmitshur added this to the Backlog milestone Jul 1, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Jul 1, 2020

/cc @matloob @jayconrod @bcmills per owners.

@matloob
Copy link
Contributor

matloob commented Jul 1, 2020

I'd really like for this to happen. We'll have to find out not only how to propagate the overlaid file contents through cmd/go, but also to the compiler...

@stamblerre
Copy link
Contributor Author

@matloob: should this be added to the 1.16 milestone now?

@matloob matloob modified the milestones: Backlog, Go1.16 Aug 27, 2020
@matloob
Copy link
Contributor

matloob commented Aug 27, 2020

yes. done.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/253747 mentions this issue: cmd/go: add basic support for overlays

@matloob matloob self-assigned this Sep 9, 2020
@matloob matloob removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 9, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/257198 mentions this issue: cmd/go: rewrite paths for overlaid files using -trimpath

gopherbot pushed a commit that referenced this issue Oct 6, 2020
This CL adds basic support for listing packages with overlays.
The new cmd/go/internal/fs package adds an abstraction for communicating
with the file system that will open files according to their overlaid paths,
and provides functions to override those in the build context to open
overlaid files. There is also some support for executing builds on packages
with overlays. In cmd/go/internal/work.(*Builder).build, paths are mapped
to their overlaid paths before they are given as arguments to tools.

For #39958

Change-Id: I5ec0eb9ebbca303e2f1e7dbe22ec32613bc1fd17
Reviewed-on: https://go-review.googlesource.com/c/go/+/253747
Trust: Michael Matloob <matloob@golang.org>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@matloob
Copy link
Contributor

matloob commented Oct 7, 2020

To provide some more detail on how this will show up in the cmd/go interface, we'll add a -overlay flag to cmd/go. Overlay will take a path to a file containing JSON-serialized struct that specifies the overlay. The struct is defined as:

// OverlayJSON is the format overlay files are expected to be in.
// The Replace map maps from overlaid paths to replacement paths:
// the Go command will forward all reads trying to open
// each overlaid path to its replacement path, or consider the overlaid
// path not to exist if the replacement path is empty.
type OverlayJSON struct {
	Replace map[string]string
}

All entries in the replace map correspond to a single file that is specified to exist in the filesystem with the path of the file given by the key and and the contents of the file given by the contents of the file with the path given by the value. The case where a value for a given key is empty means that cmd/go should consider the file given by the path in the key to not exist.

If the directory of a file path in the key doesn't exist then the cmd/go will implicitly consider that directory and its (potentially nonexistent) parent directories to exist in the overlay filesystem.

If cmd/go does a build using overlaid files (even though the purpose of the overlay support is to support go/packages and gopls), it will use trimpath so that the paths in the keys in the map show up in the export data and binaries.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/262617 mentions this issue: cmd/go: support overlays for synthesized packages.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/262618 mentions this issue: cmd/go: support cgo files in overlays

gopherbot pushed a commit that referenced this issue Oct 15, 2020
Pass the trimpath flag to cmd/compile to use the correct file paths
for files that are overlaid: that is, the "destination" path in the
overlay's Replace mapping rather than the "source" path.

Also fix paths to go source files provided to the gccgo compiler.

For #39958

Change-Id: I3741aeb2272bd0d5aa32cb28133b61e58264fd39
Reviewed-on: https://go-review.googlesource.com/c/go/+/257198
Trust: Michael Matloob <matloob@golang.org>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/263737 mentions this issue: cmd/go: dissallow overlays of non-.go and non-go.mod files

gopherbot pushed a commit that referenced this issue Oct 20, 2020
The main missing piece here was supporting Stat in the overlay
filesystem, in the parts of the package code that determines whether
an command line argument is a file on disk or a directory.  so this
change adds a Stat function to the fsys package. It's implemented the
same way as the already existing fsys.lstat function, but instead of
os.Lstat, it calls os.Stat on disk files.

Then, the change changes parts of the package code to use the overlay
Stat instead of the os package's Stat.

For #39958

Change-Id: I8e478ae386f05b48d7dd71bd7e47584f090623df
Reviewed-on: https://go-review.googlesource.com/c/go/+/262617
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/264478 mentions this issue: cmd/go: replace some more stats with fsys.Stat

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 22, 2020
gopherbot pushed a commit that referenced this issue Oct 23, 2020
To support overlays

For #39958

Change-Id: I5ffd72aeb7f5f30f6c60f6334a01a0a1383c7945
Reviewed-on: https://go-review.googlesource.com/c/go/+/264478
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/265758 mentions this issue: cmd/go: don't unnecessarily copy non-overlaid cgo files to objdir

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/266358 mentions this issue: cmd/cgo: add -trimpath flag allowing paths to be rewritten in outputs

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/266376 mentions this issue: cmd/go: pass in overlaid file paths to C compiler

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/266417 mentions this issue: cmd/go: pass in overlaid paths for .s files

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/266720 mentions this issue: cmd/go: use overlaid path contents in build cache

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/266797 mentions this issue: cmd/go: support overlaying go.mod files

gopherbot pushed a commit that referenced this issue Oct 31, 2020
cmd/cgo now has a -trimpath flag that behaves the same as the
-trimpath flag to cmd/compile. This will be used to correct paths
to cgo files that are overlaid.

The code that processes trimpath in internal/objapi has been slightly
refactored because it's currently only accessible via AbsFile, which
does some additional processing to the path names. Now an
ApplyRewrites function is exported that just applies the trimpath
rewrites.

Also remove unused srcfile argument to cmd/cgo.(*Package).godefs.

For #39958

Change-Id: I497d48d0bc2fe1f6ab2b5835cbe79f15b839ee59
Reviewed-on: https://go-review.googlesource.com/c/go/+/266358
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Oct 31, 2020
This requires rewriting the paths of the files passed to the cgo tool
toolchain to use the overlaid paths instead of the disk paths of
files. Because the directories of the overlaid paths don't exist in
general, the cgo tool have been updated to run in base.Cwd instead of
the package directory.

For #39958

Change-Id: I8986de889f56ecc2e64fa69f5f6f29fa907408f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/262618
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/267197 mentions this issue: cmd/go: support cgo files in overlays

gopherbot pushed a commit that referenced this issue Nov 2, 2020
This is a roll-forward of golang.org/cl/262618, which was reverted in
golang.org/cl/267037. The only differences between this CL and the
original are the three calls to fflush from the C files in
build_overlay.txt, to guarantee that the string we're expecting is
actually written out.

This requires rewriting the paths of the files passed to the cgo tool
toolchain to use the overlaid paths instead of the disk paths of
files. Because the directories of the overlaid paths don't exist in
general, the cgo tool have been updated to run in base.Cwd instead of
the package directory.

For #39958

Change-Id: If7e5e057c62c0c22ddb724f9fe650902fc5f4832
Reviewed-on: https://go-review.googlesource.com/c/go/+/267197
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Michael Matloob <matloob@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/267797 mentions this issue: cmd/go: support cgo files in overlays

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/267877 mentions this issue: cmd/go: account for flags when parsing regexps in TestScript

gopherbot pushed a commit that referenced this issue Nov 5, 2020
Test script expects the regexp argument for stdout, stderr, and cmp
to be the first argument after the command, but that might not be the
case if the -q or -count flags are provided. Treat the first argument
after a flag as a regexp instead.

For #39958

Change-Id: I369926109ec10cca8b2c3baca27e7a3f7baf364b
Reviewed-on: https://go-review.googlesource.com/c/go/+/267877
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Nov 9, 2020
This is a roll-forward of golang.org/cl/267197, which was reverted in
golang.org/cl/267357. It makes the following changes in addition to
the ones in the next paragraph: It avoids outputting trimpath
arguments for an overlay unless the overlay affects the package being
compiled (to avoid hitting windows command line argument limits), and
it fixes processing of regexps in the script test framework to treat
the first *non flag* argument to grep, stdout, and stderr as a regexp,
not just the first argument.

golang.org/cl/267917 was a roll-forward of golang.org/cl/262618, which
was reverted in golang.org/cl/267037. The only differences between
this CL and the original were the three calls to fflush from the C
files in build_overlay.txt, to guarantee that the string we were
expecting was
actually written out.

The CL requires rewriting the paths of the files passed to the cgo
tool toolchain to use the overlaid paths instead of the disk paths of
files. Because the directories of the overlaid paths don't exist in
general, the cgo tool have been updated to run in base.Cwd instead of
the package directory.

For #39958

Change-Id: I1bd96db257564bcfd95b3502aeca14d04bd28618
Reviewed-on: https://go-review.googlesource.com/c/go/+/267797
Trust: Michael Matloob <matloob@golang.org>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Nov 10, 2020
The previous cl (golang.org/cl/262618) copied non-overlaid cgo files
to objdir, mostly to get around the issue that otherwise cgo-generated
files were written out with the wrong names (they'd get the base path
of the overlay file containing the replaced contents, instead of the
base path of the path whose contents are being replaced). So that CL
it would copy the files to objdir with the base path of the file
being replaced to circumvent that.

This CL changes cmd/go and cmd/cgo so that instead of copying
files, it passes the actual path of the file on disk either of
the original file (if it is not overlaid) or its replacement
file (if it is) as well as a flag --path_rewrite, newly added to
cmd/cgo, that specifies the actual original file path that corresponds
to the replaced files.

Updates #39958

Change-Id: Ic4aae5ef77fe405011fcdce7f6c162488d13daa2
Reviewed-on: https://go-review.googlesource.com/c/go/+/265758
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/269017 mentions this issue: cmd/go: don't copy cgo files to objdir when overlay is present

gopherbot pushed a commit that referenced this issue Nov 11, 2020
This cl is a roll-forward of golang.org/cl/265758, which was rolled back
in golang.org/cl/268900. The changes made are removing cgofiles
from the list of files that are copied to objdir (because the cgofiles
themselves aren't actually provided to the compiler) and fixing test
cases to properly provide the overlay flag and to allow for paths with
backslashes (as in Windows).

The previous cl (golang.org/cl/262618) copied non-overlaid cgo files
to objdir, mostly to get around the issue that otherwise cgo-generated
files were written out with the wrong names (they'd get the base path
of the overlay file containing the replaced contents, instead of the
base path of the path whose contents are being replaced). So that CL
it would copy the files to objdir with the base path of the file
being replaced to circumvent that.

This CL changes cmd/go and cmd/cgo so that instead of copying
files, it passes the actual path of the file on disk either of
the original file (if it is not overlaid) or its replacement
file (if it is) as well as a flag --path_rewrite, newly added to
cmd/cgo, that specifies the actual original file path that corresponds
to the replaced files.

Updates #39958

Change-Id: Ia45b022f9d27cfce0f9ec6da5f3a9f53654c67b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/269017
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 12, 2020
This change moves the code in work.(*Builder).cgo that, when there is
an overlay, copies non-Go files to objdir into work.(*Builder).Build,
and creates an overlay structure mapping from the nominal file paths
into the copies in objdir. That's propagated through to
work.(*Builder).ccompile, which will use it to pass in the path to the
overlaid contents in objdir when calling the compiler.

This allows for overlays of C/C++/Fortran files.

For #39958

Change-Id: I9a2e3d3ba6afdf7ce19be1dbf4eee34805cdc05f
Reviewed-on: https://go-review.googlesource.com/c/go/+/266376
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Nov 12, 2020
This change adds support for adding overlays on assembly files.

For #39958

Change-Id: I1a328656199cc836f48e16de1ffd944fdd07fb39
Reviewed-on: https://go-review.googlesource.com/c/go/+/266417
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 12, 2020
When caching actions, use the overlaid file contents, because those
are the ones actually used to produce the outputs.

For #39958

Change-Id: Ia1f85b2fcf1f26e3b5be82f4d35c2726b134a36b
Reviewed-on: https://go-review.googlesource.com/c/go/+/266720
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 20, 2020
This change updates the lockedfile package to open files using the
new fsys.OpenFile function. The logic of fsys.Open has been moved into
fsys.OpenFile, and fsys.Open is now just a light wrapper around it.

For #39958

Change-Id: I552f1a45ac00ac06b5812008d17a61e610b4b113
Reviewed-on: https://go-review.googlesource.com/c/go/+/266797
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/272126 mentions this issue: cmd/go: support the -overlay flag for go mod commands

gopherbot pushed a commit that referenced this issue Nov 20, 2020
Move the declaration of the -overlay flag to base.AddModCommonFlags,
where other flags that are needed for go mod commands and for builds
are declared. The flag's already initialized in modload.Init so
there's no additional work needed to be done to support it in the go
mod commands.

For #39958

Change-Id: I70725d620cc69cb820f6ed923d626f4fe041b1c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/272126
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/274714 mentions this issue: cmd/go: add documentation for the -overlay flag

@dolmen
Copy link
Contributor

dolmen commented Feb 15, 2021

I hope that this feature will not be released in its current state with Go 1.16.

@dolmen
Copy link
Contributor

dolmen commented Feb 15, 2021

The documentation for the -overlay flag from go1.16rc1 help build says:

  Support for the -overlay flag
  has some limitations:importantly, cgo files included from outside the
  include path must be  in the same directory as the Go package they are
  included from, and overlays will not appear when binaries and tests are
  run through go run and go test respectively.

It isn't clear to me what means "overlays will not appear". And is it only related to cgo files?

@matloob
Copy link
Contributor

matloob commented Feb 16, 2021

I'll try to update the docs, but essentially, when using go run and go test, if the running binary tries to open files, the overlay will not affect the presence of files or their contents.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/292710 mentions this issue: cmd/go: update -overlay docs to make its purpose more clear

@golang golang locked and limited conversation to collaborators Feb 16, 2022
@rsc rsc unassigned matloob Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants
@dolmen @dmitshur @stamblerre @gopherbot @matloob and others