Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

Modules and imports in the Overlay, also all inputs from overlay #607

Closed
verdverm opened this issue Dec 7, 2020 · 8 comments
Closed

Modules and imports in the Overlay, also all inputs from overlay #607

verdverm opened this issue Dec 7, 2020 · 8 comments
Labels
FeatureRequest New feature or request module/imports

Comments

@verdverm
Copy link
Contributor

verdverm commented Dec 7, 2020

Is your feature request related to a problem? Please describe.

I would like to embed a module into the overlay. This would be useful for shipping binaries with the schema embedded, especially with go 1.16 supporting file embeds natively.

This currently does not work due to how the overlay and cue.mod/module.cue is handled in cue/load/...

Another use case would be loading all inputs from the Overlay. This is useful in a server context where one wants to avoid disk usage. This seems to not work because of how the args to load.Instances works. It seems one cannot specify files from the Overlay there.

Am I missing a configuration setting?

Describe the solution you'd like

The ability to do the above. Guidance and input on implementation.

Additional context

I've traced the first use case to some TODOs in the code. The first is here: https://github.com/cuelang/cue/blob/master/cue/load/config.go#L498

The processing in this area generates absolute paths. The fileSystem in fs.go https://github.com/cuelang/cue/blob/master/cue/load/fs.go#L206

With Go 1.16, there is a new FS interface, maybe that should be used? If so, maybe that means this feature is delayed until 1.16 is more widely adopted?


Example that is not working as desired or expected:

package main

import (
        "bytes"
        "fmt"
        "os"

        "cuelang.org/go/cue"
        "cuelang.org/go/cue/load"
)

func check(err error) {
        if err != nil {
                fmt.Println(err)
                os.Exit(1)
        }
}

var stdin string = `
package a

hello: "world"

foo: A
`

func main() {
        var b bytes.Buffer
        var r   cue.Runtime

        fmt.Fprintln(&b, stdin)

        addOverlay()
        config.Package = "*"
        config.Stdin = &b
        config.ModuleRoot = "./"
        config.Dir = "./"
        fmt.Println("Overlay:", config.Overlay)

        bis := load.Instances([]string{"-"}, &config)
        for _, bi := range bis {
                check(bi.Err)

                I, err := r.Build(bi)
                check(err)

                v := I.Value()
                fmt.Printf("%v\n", v)
        }
}

func addOverlay() {
        for fn, src := range files {
                config.Overlay[fn] = load.FromString(src)
        }
}

var config load.Config = load.Config {
        Overlay: map[string]load.Source{},
}

var files = map[string]string{
        "cue.mod/module.cue": `
module: "hof.io/foo/bar"
`,

        "a.cue": `
package a

A: "A"
`,

        "b/b.cue": `
package b

B: "B"
`,
}
@verdverm verdverm added the FeatureRequest New feature or request label Dec 7, 2020
@mpvl
Copy link
Contributor

mpvl commented Dec 8, 2020

I'm not entirely sure I understand what you're asking. But load should read all files to its internal fs which uses Overlay. So that includes module files.

It would be useful to use embedding and fs. But we aim to support at least 2 versions back, so that will take a while. Either way, these are implementation details, and overlays should work for all files. It could be that it requires a directory that is overlayed to exist, but also that could be tweaked. I'm not sure what you desired or expected here, so I can't tell what is wrong easily.

@verdverm
Copy link
Contributor Author

verdverm commented Dec 8, 2020

I'm seeing issues with the overlay, I think because of the makeAbs function, though not confirmed yet.

I've added some printing, working on a test file and setup, will push something to gerrit later

Overlay: map[a.cue:
package a

A: "A"
 b/b.cue:
package b

B: "B"
 cue.mod/module.cue:
module: "cuelang.org/foo/bar"
]
config.fs.init - start
overlay: a.cue
overlay: b/b.cue
overlay: cue.mod/module.cue
config.fs.init - initd
config.fs.complete - cue.mod dirname cue.mod
config.fs.complete - cue.mod error stat: stat /home/tony/cue/gerrit/cue/load/testdata/overlay/cue.mod: no such file or directory
module root not defined%!(EXTRA string=.)
exit status 1

I've tried various settings for config.ModuleRoot and config.Dir and they end in various errors or unexpected output.

@verdverm
Copy link
Contributor Author

verdverm commented Dec 9, 2020

I think I found the issue:

  1. https://github.com/cuelang/cue/blob/master/cue/load/config.go#L483
  2. https://github.com/cuelang/cue/blob/master/cue/load/fs.go#L64
  3. https://github.com/cuelang/cue/blob/master/cue/load/fs.go#L122
  • At (1) c.Dir will always become an absolute path
  • At (2) f.cwd will always get this same absolute path
  • At (3) fs.makeAbs will always be a real external FS absolute path

Now, since almost all fileSystem functions use fs.makeAbs on their first line, there is almost no way to get into the overlay, which should only have non-absolute paths.

...unless the users should be placing / in front of every filename key they add as a Overlay source. If this is the case, we ought to

  • make note in the docs that this is the case
  • and one of the following
    • return an error if filename provided as a key is not absolute
    • insert a / as a prefix

But, given the comment here: https://github.com/cuelang/cue/blob/master/cue/load/config.go#L263
... a users would assume that the file would be give as a relative path so that the Overlay replaces the relative file on disk. This is how I read it anyhow


config settings

        config.Package = "a"
        config.Stdin = &b
        config.ModuleRoot = "./"
                                // config.Module = "cuelang.org/foo/bar"
        config.Dir = ""

go run output

Overlay: map[a.cue:
package a

A: "A"
 b/b.cue:
package b

B: "B"
 cue.mod/module.cue:
module: "cuelang.org/foo/bar"
]
config.fs.init - start
fs.init load.Config{Context:(*build.Context)(nil), loader:(*load.loader)(nil), ModuleRoot:"./", Module:"", Package:"a", Dir:"/home/tony/cue/gerrit/cue/load/testdata/overlay", Tags:[]string(nil), AllCUEFiles:false, BuildTags:[]string(nil), releaseTags:[]string{"cue0.1"}, Tests:false, Tools:false, filesMode:false, DataFiles:false, StdRoot:"", ParseFile:(func(string, interface {}) (*ast.File, error))(nil), Overlay:map[string]load.Source{"a.cue":"\npackage a\n\nA: \"A\"\n", "b/b.cue":"\npackage b\n\nB: \"B\"\n", "cue.mod/module.cue":"\nmodule: \"cuelang.org/foo/bar\"\n"}, Stdin:(*bytes.Buffer)( 0xc0002dfce0), fileSystem:load.fileSystem{overlayDirs:map[string]map[string]*load.overlayFile(nil), cwd:""}, loadFunc:(build.LoadFunc)(nil)}
processing "cue.mod/module.cue" from overlay
processing "a.cue" from overlay
processing "b/b.cue" from overlay
config.fs.init - initd
config.fs.complete - cue.mod dirname cue.mod
fs.stat - input cue.mod
fs.makeAbs cwd:(/home/tony/cue/gerrit/cue/load/testdata/overlay) "cue.mod" --(n/a)--> "/home/tony/cue/gerrit/cue/load/testdata/overlay/cue.mod"
fs.stat - abs /home/tony/cue/gerrit/cue/load/testdata/overlay/cue.mod
fs.getOverlay /home/tony/cue/gerrit/cue/load/testdata/overlay/cue.mod
fs.getOverlay - split /home/tony/cue/gerrit/cue/load/testdata/overlay/ cue.mod
fs.getOverlay - not found /home/tony/cue/gerrit/cue/load/testdata/overlay/cue.mod
config.fs.complete - cue.mod error stat: stat /home/tony/cue/gerrit/cue/load/testdata/overlay/cue.mod: no such file or directory
module root not defined "."
exit status 1

@tonyhb
Copy link

tonyhb commented Feb 21, 2021

Using Overlay with 1.16's FS to embed my packages in my binary and ran into this. Also notice that there are a few filesystem requests when loading packages even if they're in overlay within a Config.

cuelang.org/go/cue/load/import.go's GenPath will attempt to load data from the filesystem - even if all of the Cue files are within the in-memory overlay buffer.

This breaks a very specific flow: I'm compiling a cue validation binary to WASM, with all cue files in memory via Overaly. However, because import.go always accesses the disk modules break and cannot be used in a webassembly build.

tonyhb added a commit to tonyhb/cue that referenced this issue Feb 21, 2021
`fileSystem.readDir` always reads from the actual filesystem, even if
there is an overaly directory present.  Read from the overlay directory
in these circumstances;  we shouldn't be hitting the disk.

Also, if the root path is "/" we can skip loading and return true.  This
allows fully in-memory packages as per
cuelang#607.
@verdverm
Copy link
Contributor Author

There are other places where Cue uses the local FS. In particular, this was a recent bugfix: a616925

Cue has a WASM playground, maybe that could help as a reference. The repo for that part is here: https://github.com/cue-sh/playground

@tonyhb
Copy link

tonyhb commented Feb 21, 2021

Yes, but in particular using overlay in a wasm environment causes an an error here: https://github.com/cuelang/cue/pull/786/files#diff-67213f39b84a013e7db04664cd3ad943b7a237d97b9c6e862d10806ac27def28R418-R420 :)

There are several places where a filesystem is still expected even after that bugfix. The PR above fixes the ones that I've ran into in the latest beta. It (should) also enable module imports from overlays by fixing areas of the filesystem code that didn't inspect overlays as it probably should.

@myitcv
Copy link
Contributor

myitcv commented Mar 13, 2021

Building on my comment in #786 (comment), like @mpvl I'm neither entirely clear what's being asked for, nor do I understand what bug is being reported in this issue. That said, thanks @verdverm and @tonyhb for looking into this, because I think there is a gap here that is nicely filled by io/fs.FS.

The docs for Overlay currently state the following:

	// Overlay provides a mapping of absolute file paths to file contents.
	// If the file with the given path already exists, the parser will use the
	// alternative file contents provided by the map.
	//
	// If the value must be of type string, []byte, io.Reader, or *ast.File.

(I've just mailed https://cue-review.googlesource.com/c/cue/+/9021 to remove that last out-of-date paragraph)

Put another way, an overlay, in its current form, exists in order to complement what is on disk, rather than wholesale replace its contents.

Therefore I'm unclear that the following expectation is correct:

Read from the overlay directory in these circumstances; we shouldn't be hitting the disk.

Indeed it's not possible AFAIU to have an Overlay in its current form either complement what is on disk and act as a substitute for it - we would need another "switch" to say what mode we want.

But as I mentioned I think such a "switch" is unnecessary - we can instead add an io/fs.FS-typed field to cue/load.Config: if provided, we walk that file system, otherwise we behave as we do today.

We should as, part of this change, decide what to do with Overlay. Because in truth with an io/fs.FS, Overlay becomes redundant. As such we should look to make a breaking change pre v1.0.0 to remove it when we can.

We could safely make this change today, guarded by build tags - go1.16 and !go1.16 - and then simply drop the !go1.16 code once we no longer support < go1.16. The overhead of maintaining "dual" versions of cue/load.Config and some associated code would be time-bounded, and would be cleanly achieved by having separate files for the two.

Thoughts on that as a way forward?

If so, we would like help implementing this. The only gentle request being that we make this change via Gerrit (because discussion about code, review etc is much easier and more productive).

@cueckoo
Copy link

cueckoo commented Jul 3, 2021

This issue has been migrated to cue-lang/cue#607.

For more details about CUE's migration to a new home, please see cue-lang/cue#1078.

@cueckoo cueckoo closed this as completed Jul 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest New feature or request module/imports
Projects
None yet
Development

No branches or pull requests

5 participants