Skip to content

Commit

Permalink
cue/load: do not create packages unnecessarily for files outside pack…
Browse files Browse the repository at this point in the history 
…age directory

The current logic creates instances for files outside a package's
directory even when those have nothing to do with the package in
question.

Change the logic so that when a file in a parent directory is added, an
instance is only created when that file's package matches one of the
packages in the original directory.

Note: this is only an issue when `PkgName` is set to `*` to load all
packages.

Note also: this fixes some of the existing behavior, as witness the
other test result changes that mention `anon.cue` where that file is
actually irrelevant and should not be included as part of the instance
(packageless CUE files never include packageless CUE files from parent
directories).

Fixes #3306

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I7f158b70f5d64068aa031b063aaca5680d738132
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1198686
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
  • Loading branch information
@rogpeppe
rogpeppe committed Jul 31, 2024
1 parent f472fd2 commit a4abb05
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 60 deletions.
12 changes: 11 additions & 1 deletion cue/load/import.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ import (
// is present.
// _ anonymous files (which may be marked with _)
// * all packages
func (l *loader) importPkg(pos token.Pos, p *build.Instance) (_ret []*build.Instance) {
func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {
retErr := func(errs errors.Error) []*build.Instance {
// XXX: move this loop to ReportError
for _, err := range errors.Errors(errs) {
Expand Down Expand Up @@ -192,6 +192,16 @@ func (l *loader) importPkg(pos token.Pos, p *build.Instance) (_ret []*build.Inst
p.ReportError(errors.Promote(err, ""))
}

if len(p.BuildFiles) == 0 &&
len(p.IgnoredFiles) == 0 &&
len(p.OrphanedFiles) == 0 &&
len(p.InvalidFiles) == 0 &&
len(p.UnknownFiles) == 0 {
// The package has no files in it. This can happen
// when the default package added in newFileProcessor
// doesn't have any associated files.
continue
}
all = append(all, p)
rewriteFiles(p, cfg.ModuleRoot, false)
if errs := fp.finalize(p); errs != nil {
Expand Down
36 changes: 31 additions & 5 deletions cue/load/loader_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,14 @@ func (fp *fileProcessor) add(root string, file *build.File, mode importMode) {
// special * and _
p := fp.pkg // default package

// sameDir holds whether the file should be considered to be
// part of the same directory as the default package. This is
// true when the file is part of the original package directory
// or when allowExcludedFiles is specified, signifying that the
// file is part of an explicit set of files provided on the
// command line.
sameDir := filepath.Dir(fullPath) == p.Dir || (mode&allowExcludedFiles) != 0

// badFile := func(p *build.Instance, err errors.Error) bool {
badFile := func(err errors.Error) {
fp.err = errors.Append(fp.err, err)
Expand All @@ -181,7 +189,9 @@ func (fp *fileProcessor) add(root string, file *build.File, mode importMode) {

if file.Encoding != build.CUE {
// Not a CUE file.
p.OrphanedFiles = append(p.OrphanedFiles, file)
if sameDir {
p.OrphanedFiles = append(p.OrphanedFiles, file)
}
return
}
if (mode & allowExcludedFiles) == 0 {
Expand All @@ -192,6 +202,9 @@ func (fp *fileProcessor) add(root string, file *build.File, mode importMode) {
}
}
if badPrefix != "" {
if !sameDir {
return
}
file.ExcludeReason = errors.Newf(token.NoPos, "filename starts with a '%s'", badPrefix)
if file.Interpretation == "" {
p.IgnoredFiles = append(p.IgnoredFiles, file)
Expand All @@ -217,9 +230,18 @@ func (fp *fileProcessor) add(root string, file *build.File, mode importMode) {
pos := pf.Pos()

switch {
case pkg == p.PkgName, mode&allowAnonymous != 0:
case pkg == p.PkgName && (sameDir || pkg != "_"):
// We've got the exact package that's being looked for.
// It will already be present in fp.pkgs.
case mode&allowAnonymous != 0 && sameDir:
// It's an anonymous file that's not in a parent directory.
case fp.allPackages && pkg != "_":
q := fp.pkgs[pkg]
if q == nil && !sameDir {
// It's a file in a parent directory that doesn't correspond
// to a package in the original directory.
return
}
if q == nil {
q = &build.Instance{
PkgName: pkg,
Expand All @@ -235,10 +257,14 @@ func (fp *fileProcessor) add(root string, file *build.File, mode importMode) {
p = q

case pkg != "_":

// We're loading a single package and we either haven't matched
// the earlier selected package or we haven't selected a package
// yet. In either case, the default package is the one we want to use.
default:
file.ExcludeReason = excludeError{errors.Newf(pos, "no package name")}
p.IgnoredFiles = append(p.IgnoredFiles, file)
if sameDir {
file.ExcludeReason = excludeError{errors.Newf(pos, "no package name")}
p.IgnoredFiles = append(p.IgnoredFiles, file)
}
return
}

Expand Down
58 changes: 4 additions & 54 deletions cue/load/loader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,6 @@ files:
},
args: []string{"./toolonly"},
want: `err: build constraints exclude all CUE files in ./toolonly:
anon.cue: no package name
test.cue: package is test, want foo
toolonly/foo_tool.cue: _tool.cue files excluded in non-cmd mode
path: mod.test/test/toolonly@v0:foo
Expand Down Expand Up @@ -499,7 +498,6 @@ root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/multi4
display:.
files:
$CWD/testdata/testmod/anon.cue
$CWD/testdata/testmod/multi4/nopackage1.cue
$CWD/testdata/testmod/multi4/nopackage2.cue`}, {
// Test what happens when there's a single CUE file
Expand All @@ -516,7 +514,6 @@ root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/multi5
display:.
files:
$CWD/testdata/testmod/anon.cue
$CWD/testdata/testmod/multi5/nopackage.cue`}, {
// Check that imports are only considered from files
// that match the build paths.
Expand Down Expand Up @@ -570,42 +567,19 @@ display:""`}, {
// in the result of Instances.
name: "Issue3306",
cfg: &Config{
Dir: testdataDir,
Package: "*",
Dir: testdataDir,
Package: "*",
SkipImports: true,
},
args: []string{"./issue3306/..."},
want: `path: mod.test/test/issue3306@v0:_
module: mod.test/test@v0
root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/issue3306
display:./issue3306
files:
$CWD/testdata/testmod/anon.cue
path: mod.test/test/issue3306@v0:test
module: mod.test/test@v0
root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/issue3306
display:./issue3306
files:
$CWD/testdata/testmod/test.cue
path: mod.test/test/issue3306@v0:x
want: `path: mod.test/test/issue3306@v0:x
module: mod.test/test@v0
root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/issue3306
display:./issue3306
files:
$CWD/testdata/testmod/issue3306/x.cue
path: mod.test/test/issue3306/a@v0:_
module: mod.test/test@v0
root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/issue3306/a
display:./issue3306/a
files:
$CWD/testdata/testmod/anon.cue
path: mod.test/test/issue3306/a@v0:a
module: mod.test/test@v0
root: $CWD/testdata/testmod
Expand All @@ -622,14 +596,6 @@ display:./issue3306/a
files:
$CWD/testdata/testmod/issue3306/a/b.cue
path: mod.test/test/issue3306/a@v0:test
module: mod.test/test@v0
root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/issue3306/a
display:./issue3306/a
files:
$CWD/testdata/testmod/test.cue
path: mod.test/test/issue3306/a@v0:x
module: mod.test/test@v0
root: $CWD/testdata/testmod
Expand All @@ -639,22 +605,6 @@ files:
$CWD/testdata/testmod/issue3306/x.cue
$CWD/testdata/testmod/issue3306/a/x.cue
path: mod.test/test/issue3306/x@v0:_
module: mod.test/test@v0
root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/issue3306/x
display:./issue3306/x
files:
$CWD/testdata/testmod/anon.cue
path: mod.test/test/issue3306/x@v0:test
module: mod.test/test@v0
root: $CWD/testdata/testmod
dir: $CWD/testdata/testmod/issue3306/x
display:./issue3306/x
files:
$CWD/testdata/testmod/test.cue
path: mod.test/test/issue3306/x@v0:x
module: mod.test/test@v0
root: $CWD/testdata/testmod
Expand Down

0 comments on commit a4abb05

Please sign in to comment.