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/cue: regression with CUE_EXPERIMENT=modules where unnamed files are also loaded #3144

Closed
mvdan opened this issue May 14, 2024 · 0 comments
Labels
modules Issues related to CUE modules and the experimental implementation

Comments

@mvdan
Copy link
Member

mvdan commented May 14, 2024

$ cue version
cue version v0.9.0-alpha.4.0.20240514085722-e9bf33c50d3f

go version devel go1.23-7f9edb4225 2024-05-11 20:38:24 +0000
      -buildmode exe
       -compiler gc
        -ldflags -w -s
  DefaultGODEBUG asynctimerchan=1,gotypesalias=0,httplaxcontentlength=1,httpmuxgo121=1,tls10server=1,tlsrsakex=1,tlsunsafeekm=1,winreadlinkvolume=0,winsymlink=0
     CGO_ENABLED 1
          GOARCH amd64
            GOOS linux
         GOAMD64 v1
cue.lang.version v0.9.0
$ cat repro-cmd.txtar 
exec cue export valid.cue
cmp stdout stdout.golden

env CUE_EXPERIMENT=modules

exec cue export valid.cue
cmp stdout stdout.golden

-- stdout.golden --
{
    "out": "bar"
}
-- cue.mod/module.cue --
module: "foo.test/bar"
language: version: "v0.9.0"
-- valid.cue --
#foo: "bar"
out: #foo
-- invalid.cue --
foo :: "bar"
out: foo
$ testscript -v repro-cmd.txtar 
[...]
> exec cue export valid.cue
[stdout]
{
    "out": "bar"
}
> cmp stdout stdout.golden
> env CUE_EXPERIMENT=modules
> exec cue export valid.cue
[stderr]
instance: cannot enumerate all module imports: cannot read "invalid.cue": expected operand, found ':'
[exit status 1]
FAIL: /tmp/testscript2634378284/repro-cmd.txtar/script.txtar:6: unexpected command failure
error running repro-cmd.txtar in /tmp/testscript2634378284/repro-cmd.txtar

This appears to be some sort of bug in cmd/cue or cue/load - when naming a single file to load, we shouldn't be parsing other files in the same directory at all. This regression was spotted via Unity.

@mvdan mvdan added the modules Issues related to CUE modules and the experimental implementation label May 14, 2024
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently when files are specified on the command line,
they're considered after all packages have been evaluated
with respect to modules. We need to change that so that
we consider imports from files before creating the loader,
which requires factoring the `cueFilesPackage` method
out from `loader`.

This change makes some preparations for that by:
- parsing file arguments before invoking `loadPackages`
- passing the `filesMode` boolean as an argument rather
than indirectly in `Config`, where it doesn't really feel
like it belongs anyway, as `Config` isn't really the right
place for values that change over time.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: If6c342757e97a3752edb3fefea64ad4bdfdbe23f
cueckoo pushed a commit that referenced this issue May 15, 2024
The argument isn't used, so remove it.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I4d8c101f1523a89b54bf82a24c21226fe7176d72
cueckoo pushed a commit that referenced this issue May 15, 2024
We need to be able to read the file syntax
before creating the loader so we can determine
imports from command line files, but currently the syntax
cache is only created as part of the loader.

This CL factors out the syntax cache into its own
value so it can be used independently of the loader.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I2845406e7006a2ff9cc0b6d5dbb59798a8b1b30d
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

As the `*build.File` source is now considered twice,
we need to fill it out earlier (the `setFileSource` function)
and we also need to avoid reading the standard input
twice, so implement a once-only-reader to avoid
that.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

As the `*build.File` source is now considered twice,
we need to fill it out earlier (the `setFileSource` function)
and we also need to avoid reading the standard input
twice, so implement a once-only-reader to avoid
that.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

As the `*build.File` source is now considered twice,
we need to fill it out earlier (the `setFileSource` function)
and we also need to avoid reading the standard input
twice, so implement a once-only-reader to avoid
that.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently when files are specified on the command line,
they're considered after all packages have been evaluated
with respect to modules. We need to change that so that
we consider imports from files before creating the loader,
which requires factoring the `cueFilesPackage` method
out from `loader`.

This change makes some preparations for that by:
- parsing file arguments before invoking `loadPackages`
- passing the `filesMode` boolean as an argument rather
than indirectly in `Config`, where it doesn't really feel
like it belongs anyway, as `Config` isn't really the right
place for values that change over time.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: If6c342757e97a3752edb3fefea64ad4bdfdbe23f
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194761
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
cueckoo pushed a commit that referenced this issue May 15, 2024
The argument isn't used, so remove it.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I4d8c101f1523a89b54bf82a24c21226fe7176d72
cueckoo pushed a commit that referenced this issue May 15, 2024
We need to be able to read the file syntax
before creating the loader so we can determine
imports from command line files, but currently the syntax
cache is only created as part of the loader.

This CL factors out the syntax cache into its own
value so it can be used independently of the loader.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I2845406e7006a2ff9cc0b6d5dbb59798a8b1b30d
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

As the `*build.File` source is now considered twice,
we need to fill it out earlier (the `setFileSource` function)
and we also need to avoid reading the standard input
twice, so implement a once-only-reader to avoid
that.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
cueckoo pushed a commit that referenced this issue May 15, 2024
The argument isn't used, so remove it.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I4d8c101f1523a89b54bf82a24c21226fe7176d72
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194763
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
cueckoo pushed a commit that referenced this issue May 15, 2024
We need to be able to read the file syntax
before creating the loader so we can determine
imports from command line files, but currently the syntax
cache is only created as part of the loader.

This CL factors out the syntax cache into its own
value so it can be used independently of the loader.

For #3144
For #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I2845406e7006a2ff9cc0b6d5dbb59798a8b1b30d
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194764
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

As the `*build.File` source is now considered twice,
we need to fill it out earlier (the `setFileSource` function)
and we also need to avoid reading the standard input
twice, so implement a once-only-reader to avoid
that.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

As the `*build.File` source is now considered twice,
we need to fill it out earlier (the `setFileSource` function)
and we also need to avoid reading the standard input
twice, so implement a once-only-reader to avoid
that.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
cueckoo pushed a commit that referenced this issue May 15, 2024
Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules Issues related to CUE modules and the experimental implementation
Projects
Archived in project
Status: v0.9.0-alpha.5
Development

No branches or pull requests

1 participant