Skip to content

Commit

Permalink
cue/load: evaluate only those packages which are needed
Browse files Browse the repository at this point in the history
DO NOT REVIEW

This change changes the cue/load logic to expand package
wildcards before invoking the `modpkgload.LoadPackages`
resolution logic. This involves moving the wildcard expansion
code into a new place, independent of the `cue/load.loader`
which is created after doing that. We use the `modimports.AllModuleFiles`
logic to enumerate the packages matched by a wildcard.

All tests pass apart from the ones involving `cue import`,
which expose a significant flaw in the above approach:
`modimports.AllModuleFiles` only looks for CUE files, but
`cue import` relies on the fact that package patterns will currently
match directories that do not contain any CUE files.

Fixes #3155

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I9f4b210eb0588e21ae942a97d7cf34c9887363dc
  • Loading branch information
rogpeppe committed May 24, 2024
1 parent d66eede commit 64ec6ca
Show file tree
Hide file tree
Showing 28 changed files with 652 additions and 243 deletions.
11 changes: 4 additions & 7 deletions cmd/cue/cmd/testdata/script/cmd_filetypes.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,9 @@ exec cue export ./somepkg/
cmp stdout export.golden
# TODO(mvdan): we should support loading packages from absolute directories.
! exec cue export ${WORK}${/}somepkg
[!windows] stderr 'no location found for package ".*somepkg"'
[windows] stderr 'no encoding specified for file ".*somepkg.*"'
stderr 'cannot use absolute directory ".*somepkg" as package path'
! exec cue export ${WORK}${/}somepkg${/}
[!windows] stderr 'no dependency found for package ".*somepkg"'
[windows] stderr 'no encoding specified for file ".*somepkg.*"'
stderr 'cannot use absolute directory ".*somepkg." as package path'

# Now do some of the same filename tests, but for a file without any extension.
cp somefile.cue somefile
Expand All @@ -54,11 +52,10 @@ stderr 'standard library import path "somefile" cannot be imported as a CUE pack

# TODO(mvdan): "cannot find package" is perhaps confusing to the user given that the file exists.
! exec cue export ./somefile
stderr 'cannot find package "\./somefile"'
stderr '\./somefile refers to a file not a package directory'
# TODO(mvdan): we probably should not behave differently on Windows.
! exec cue export ${WORK}${/}somefile
[!windows] stderr 'no location found for package ".*somefile"'
[windows] stderr 'no encoding specified for file ".*somefile"'
stderr 'cannot use absolute directory ".*somefile" as package path'

-- cue.mod/module.cue --
module: "mod.test/test"
Expand Down
4 changes: 2 additions & 2 deletions cmd/cue/cmd/testdata/script/fmt.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ cmp issue1744/invalid.cue issue1744/invalid.cue.orig

-- expect-stderr --
expected 'STRING', found '.':
./fmt/error.cue:1:9
fmt/error.cue:1:9
-- fmt/error.cue --
import a.b "foo"

Expand All @@ -21,4 +21,4 @@ bb: 3
~
-- issue1744/stderr-golden --
illegal character U+007E '~':
./issue1744/invalid.cue:1:1
./issue1744/invalid.cue:1:1
2 changes: 1 addition & 1 deletion cmd/cue/cmd/testdata/script/fmt_err.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ cmp y.cue out/y_cue
cd ..

-- non-existing-file.golden --
cannot find package "./non-existing-file"
cannot find package "mod.test/x/non-existing-file@v0": cannot find module providing package mod.test/x/non-existing-file@v0
-- cue.mod/module.cue --
module: "mod.test/x"
language: version: "v0.9.0"
Expand Down
4 changes: 3 additions & 1 deletion cmd/cue/cmd/testdata/script/issue174.txtar
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
! exec cue export ./issue174
cmp stderr expect-stderr
-- expect-stderr --
cannot enumerate all module imports: invalid import path "'foo'" in issue174/issue174.cue
cannot find package "mod.test/issue174@v0": cannot get imports: invalid import path "'foo'" in issue174/issue174.cue
-- cue.mod/module.cue --
module: "mod.test"
language: version: "v0.9.0"
-- issue174/issue174.cue --
package issue174

import 'foo'

a: 1
2 changes: 1 addition & 1 deletion cmd/cue/cmd/testdata/script/registry_lazy_config.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ cmp stdout expect-stdout
cp OTHER/main.cue main.cue
cp OTHER/cue.mod/module.cue cue.mod/module.cue
! exec cue export .
stderr '^test.org@v0: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: cannot do HTTP request: Get ".*": cannot load OCI auth configuration: invalid config file ".*config.json": decode failed: .*'
stderr '^test.org@v0:main: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: cannot do HTTP request: Get ".*": cannot load OCI auth configuration: invalid config file ".*config.json": decode failed: .*'
-- dockerconfig/config.json --
should be JSON but isn't
-- expect-stdout --
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
cmp stderr expect-stderr

-- expect-stderr --
test.org@v0: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.2: module example.com/e@v0.0.2: module not found:
test.org@v0:main: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.2: module example.com/e@v0.0.2: module not found:
./main.cue:2:8
-- main.cue --
package main
Expand Down
2 changes: 1 addition & 1 deletion cmd/cue/cmd/testdata/script/registry_nofallback.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ env CUE_REGISTRY=foo.com=$CUE_REGISTRY1,none
cmp stderr expect-stderr

-- expect-stderr --
main.org@v0: import failed: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: module not found:
main.org@v0:main: import failed: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: module not found:
./main.cue:2:8
-- cue.mod/module.cue --
module: "main.org@v0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
cmp stderr expect-stderr

-- expect-stderr --
test.org@v0: import failed: cannot find package "example.com/e": ambiguous import: found package example.com/e in multiple modules:
test.org@v0:main: import failed: cannot find package "example.com/e": ambiguous import: found package example.com/e in multiple modules:
example.com/e@v0 v0.0.1 (.)
local (cue.mod/pkg/example.com/e):
./main.cue:2:8
Expand Down
2 changes: 1 addition & 1 deletion cmd/cue/cmd/testdata/script/registry_wrong_prefix.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ env CUE_REGISTRY=$DEBUG_REGISTRY_HOST/something+insecure
cmp stderr expect-stderr

-- expect-stderr --
test.org@v0: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: module not found:
test.org@v0:main: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: module not found:
./main.cue:2:8
-- main.cue --
package main
Expand Down
14 changes: 7 additions & 7 deletions cue/load/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const (
moduleFile = "module.cue"
)

type importPath string

// FromArgsUsage is a partial usage message that applications calling
// FromArgs may wish to include in their -help output.
//
Expand Down Expand Up @@ -131,6 +133,7 @@ type Config struct {
// A Module is a collection of packages and instances that are within the
// directory hierarchy rooted at the module root. The module root can be
// marked with a cue.mod file.
// If this is a relative path, it will be interpreted relative to [Config.Dir].
ModuleRoot string

// Module specifies the module prefix. If not empty, this value must match
Expand Down Expand Up @@ -300,17 +303,13 @@ func (c *Config) stdin() io.Reader {
return c.Stdin
}

type importPath string

type fsPath string

func addImportQualifier(pkg importPath, name string) (importPath, errors.Error) {
func addImportQualifier(pkg importPath, name string) (importPath, error) {
if name == "" {
return pkg, nil
}
ip := module.ParseImportPath(string(pkg))
if ip.ExplicitQualifier && ip.Qualifier != name {
return "", errors.Newf(token.NoPos, "non-matching package names (%s != %s)", ip.Qualifier, name)
return "", fmt.Errorf("non-matching package names (%s != %s)", ip.Qualifier, name)
}
ip.Qualifier = name
return importPath(ip.String()), nil
Expand Down Expand Up @@ -349,7 +348,7 @@ func (c Config) complete() (cfg *Config, err error) {
if err := c.fileSystem.init(c.Dir, c.Overlay); err != nil {
return nil, err
}

// log.Printf("complete with module root %q", c.ModuleRoot)
// TODO: determine root on a package basis. Maybe we even need a
// pkgname.cue.mod
// Look to see if there is a cue.mod.
Expand Down Expand Up @@ -389,6 +388,7 @@ func (c Config) complete() (cfg *Config, err error) {
func (c *Config) loadModule() error {
// TODO: also make this work if run from outside the module?
mod := filepath.Join(c.ModuleRoot, modDir)
// log.Printf("loading module from %q", mod)
info, cerr := c.fileSystem.stat(mod)
if cerr != nil {
return nil
Expand Down
83 changes: 48 additions & 35 deletions cue/load/import.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import (
// _ anonymous files (which may be marked with _)
// * all packages
func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {
// log.Printf("importPkg %q", p.ImportPath)
retErr := func(errs errors.Error) []*build.Instance {
// XXX: move this loop to ReportError
for _, err := range errors.Errors(errs) {
Expand Down Expand Up @@ -181,7 +182,7 @@ func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {
impPath, err := addImportQualifier(importPath(p.ImportPath), p.PkgName)
p.ImportPath = string(impPath)
if err != nil {
p.ReportError(err)
p.ReportError(errors.Promote(err, ""))
}

all = append(all, p)
Expand Down Expand Up @@ -264,7 +265,6 @@ func setFileSource(cfg *Config, f *build.File) error {
}

func (l *loader) loadFunc(pos token.Pos, path string) *build.Instance {
impPath := importPath(path)
if isLocalImport(path) {
return l.cfg.newErrInstance(errors.Newf(pos, "relative import paths not allowed (%q)", path))
}
Expand All @@ -273,8 +273,10 @@ func (l *loader) loadFunc(pos token.Pos, path string) *build.Instance {
// It looks like a builtin.
return nil
}

p := l.newInstance(pos, impPath)
p := l.newInstance(pos, resolvedPackageArg{
resolved: path,
original: path,
})
_ = l.importPkg(pos, p)
return p
}
Expand All @@ -300,10 +302,21 @@ func (l *loader) newRelInstance(pos token.Pos, path, pkgName string) *build.Inst
}

dir := filepath.Join(l.cfg.Dir, filepath.FromSlash(path))
if importPath, e := l.importPathFromAbsDir(fsPath(dir), path); e != nil {
if pkgPath, e := importPathFromAbsDir(l.cfg, dir, path); e != nil {
// Detect later to keep error messages consistent.
} else {
p.ImportPath = string(importPath)
// Add package qualifier if the configuration requires it.
name := l.cfg.Package
switch name {
case "_", "*":
name = ""
}
pkgPath, e := addImportQualifier(pkgPath, name)
if e != nil {
// Detect later to keep error messages consistent.
} else {
p.ImportPath = string(pkgPath)
}
}

p.Dir = dir
Expand All @@ -320,53 +333,43 @@ func (l *loader) newRelInstance(pos token.Pos, path, pkgName string) *build.Inst
return p
}

func (l *loader) importPathFromAbsDir(absDir fsPath, key string) (importPath, errors.Error) {
if l.cfg.ModuleRoot == "" {
return "", errors.Newf(token.NoPos,
"cannot determine import path for %q (root undefined)", key)
func importPathFromAbsDir(c *Config, absDir string, origPath string) (importPath, error) {
if c.ModuleRoot == "" {
return "", fmt.Errorf("cannot determine import path for %q (root undefined)", origPath)
}

dir := filepath.Clean(string(absDir))
if !strings.HasPrefix(dir, l.cfg.ModuleRoot) {
return "", errors.Newf(token.NoPos,
"cannot determine import path for %q (dir outside of root)", key)
dir := filepath.Clean(absDir)
if !strings.HasPrefix(dir, c.ModuleRoot) {
return "", fmt.Errorf("cannot determine import path for %q (dir outside of root)", origPath)
}

pkg := filepath.ToSlash(dir[len(l.cfg.ModuleRoot):])
pkg := filepath.ToSlash(dir[len(c.ModuleRoot):])
switch {
case strings.HasPrefix(pkg, "/cue.mod/"):
pkg = pkg[len("/cue.mod/"):]
if pkg == "" {
return "", errors.Newf(token.NoPos,
"invalid package %q (root of %s)", key, modDir)
return "", fmt.Errorf("invalid package %q (root of %s)", origPath, modDir)
}

case l.cfg.Module == "":
return "", errors.Newf(token.NoPos,
"cannot determine import path for %q (no module)", key)
case c.Module == "":
return "", fmt.Errorf("cannot determine import path for %q (no module)", origPath)
default:
impPath := module.ParseImportPath(l.cfg.Module)
impPath := module.ParseImportPath(c.Module)
impPath.Path += pkg
impPath.Qualifier = ""
pkg = impPath.String()
}

name := l.cfg.Package
switch name {
case "_", "*":
name = ""
}

return addImportQualifier(importPath(pkg), name)
return importPath(pkg), nil
}

func (l *loader) newInstance(pos token.Pos, p importPath) *build.Instance {
dir, name, err := l.absDirFromImportPath(pos, p)
func (l *loader) newInstance(pos token.Pos, p resolvedPackageArg) *build.Instance {
// log.Printf("newInstance %#v", p)
dir, name, err := l.absDirFromImportPath(pos, importPath(p.resolved))
i := l.cfg.Context.NewInstance(dir, l.loadFunc)
i.Dir = dir
i.PkgName = name
i.DisplayPath = string(p)
i.ImportPath = string(p)
i.DisplayPath = p.original
i.ImportPath = p.resolved
i.Root = l.cfg.ModuleRoot
i.Module = l.cfg.Module
i.Err = errors.Append(i.Err, err)
Expand All @@ -379,7 +382,7 @@ func (l *loader) newInstance(pos token.Pos, p importPath) *build.Instance {
//
// The returned directory may not exist.
func (l *loader) absDirFromImportPath(pos token.Pos, p importPath) (absDir, name string, err errors.Error) {
dir, name, err0 := l.absDirFromImportPath1(pos, p)
dir, name, err0 := l.absDirFromImportPath1(p)
if err0 != nil {
// Any error trying to determine the package location
// is a PackageError.
Expand All @@ -388,7 +391,7 @@ func (l *loader) absDirFromImportPath(pos token.Pos, p importPath) (absDir, name
return dir, name, err
}

func (l *loader) absDirFromImportPath1(pos token.Pos, p importPath) (absDir, name string, err error) {
func (l *loader) absDirFromImportPath1(p importPath) (absDir, name string, err error) {
if p == "" {
return "", "", fmt.Errorf("empty package name in import path %q", p)
}
Expand All @@ -401,6 +404,16 @@ func (l *loader) absDirFromImportPath1(pos token.Pos, p importPath) (absDir, nam
origp := p
// Extract the package name.
parts := module.ParseImportPath(string(p))
if isLocalImport(parts.Path) {
// This can happen when there is no module.
// If there is a module, this should never happen because
// expandPackageArgs should have been used to
// turn all relative paths into absolute unambiguous paths.
if l.cfg.Module != "" {
panic(fmt.Errorf("absDirFromImportPath unexpectedly called with relative path %q", p))
}
return filepath.Join(l.cfg.Dir, parts.Path), parts.Qualifier, nil
}
name = parts.Qualifier
p = importPath(parts.Unqualified().String())
if name == "" {
Expand Down

0 comments on commit 64ec6ca

Please sign in to comment.