cue get go does not work consistently #621

cueckoo · 2021-07-03T10:30:58Z

Originally opened by @verdverm in cuelang/cue#621

ceu get go does not load its own interfaces when a go vendor directory is present and CUE is not in the dependencies.

The error is in initInterfaces() (https://github.com/cuelang/cue/blob/master/cmd/cue/cmd/get_go.go#L287)

packages.Load does not return an error, but there are errors in p[0].Errors

We should be checking for these errors where this function is used.

repro:

mkdir intstr && cd intstr
go mod init foo.bar/intstr
cue mod init foo.bar/intstr
cat << EOF > repro.go
package intstr
import (
        "k8s.io/apimachinery/pkg/util/intstr"
)
_ = intstr.IntOrString
EOF
go mod vendor
cue get go k8s.io/apimachinery/pkg/util/intstr

possible resolutions:

check for the errors and return better messages
modify the Config passed to packages.Load
embed and load the toTop and toText interfaces

(1) I'd generally prefer to have a solution that does not require the user to have Cue as a dependency. This seems like unpleasant DX, essentially telling them they need to have an artificial dependency via overly complex instructions.

(2) could be an easy solution. However, I'm leery of modifying the Config so that Cue code is fetched automatically. This might make some SecOps people unhappy / break in corp intranets.

(3) Config has an Overlay we could use to embed these files as strings. This seems like the best way to go.

We will also want to catch the errors in the other locations and provide an error message there.

What do others think?

The text was updated successfully, but these errors were encountered:

cueckoo · 2021-07-03T10:55:05Z

Original reply by @NicolasRouquette in cuelang/cue#621 (comment)

After trying a few variations, I came to the conclusion that for reproducible behavior,
it is really helpful to use 'go mod vendor'; however, in that case, we are limited to
go 1.13 to avoid this problem.

cueckoo · 2021-07-03T10:55:06Z

Original reply by @verdverm in cuelang/cue#621 (comment)

I have a fix here: https://cue-review.googlesource.com/c/cue/+/8021

Not pretty or ideal... ended up embedding and temp writing the files to disk...

Maybe some of the other Go compiler API could remove this requirement? Not sure

cueckoo · 2021-07-03T10:55:28Z

Original reply by @mpvl in cuelang/cue#621 (comment)

Interesting. Thanks for identifying the issue!

cueckoo · 2021-07-03T10:55:42Z

Original reply by @myitcv in cuelang/cue#621 (comment)

@verdverm - just so that I'm clear, can you explain from the repro above what you believe to be not working?

What version of Go are you using?

cueckoo · 2021-07-03T10:55:43Z

Original reply by @NicolasRouquette in cuelang/cue#621 (comment)

With go 1.13, following the instructions above, you get the correct behavior; that is, cue.mod/gen/k8s.io/apimachinery/pkg/util/intstr/intstr_go_gen.cue will have:

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: _

Use go > 1.13, e.g., 1.15.6, then we get instead:

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: {
        Type:   #Type  @protobuf(1,varint,opt,name=type,casttype=Type)
        IntVal: int32  @protobuf(2,varint,opt,name=intVal)
        StrVal: string @protobuf(3,bytes,opt,name=strVal)
}

This happens with cue 0.3.0-alpha6, 0.3.0-beta1 and 0.3.0-beta2
I haven't tried other versions of 0.3.0

cueckoo · 2021-07-03T10:55:44Z

Original reply by @myitcv in cuelang/cue#621 (comment)

@NicolasRouquette - and just to confirm, the ~~second~~first of these outputs is a) correct and b) what you are expecting?

Edit: critical edit

cueckoo · 2021-07-03T10:55:45Z

Original reply by @myitcv in cuelang/cue#621 (comment)

With go 1.13

We probably need to state clearly somewhere what versions of Go are supported. Is there a reason you are using Go 1.13?

(asking in the context of https://golang.org/doc/devel/release.html#policy)

cueckoo · 2021-07-03T10:55:45Z

Original reply by @myitcv in cuelang/cue#621 (comment)

Ok, here's my now complete analysis.

Example 1

For completeness, here is a testscript-style test that asserts the correct result:

exec go mod init foo.bar/intstr
exec cue mod init foo.bar/intstr
exec cue get go k8s.io/apimachinery/pkg/util/intstr
cmp cue.mod/gen/k8s.io/apimachinery/pkg/util/intstr/intstr_go_gen.cue intstr_go_gen.cue.golden

-- repro.go --
package intstr
import (
        "k8s.io/apimachinery/pkg/util/intstr"
)
_ = intstr.IntOrString
-- intstr_go_gen.cue.golden --
// Code generated by cue get go. DO NOT EDIT.

//cue:generate cue get go k8s.io/apimachinery/pkg/util/intstr

package intstr

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: _

// Type represents the stored type of IntOrString.
#Type: int64 // #enumType

#enumType:
	#Int |
	#String

#Int:    #Type & 0
#String: #Type & 1

Note this test does not include a vendor step.

Analysis based on the above:

Example 1 works with Go Version <= 1.15X by virtue of cmd/go build operations (and this includes the cmd/go list that happens as part of a go/packages.Load) automatically updating go.mod as required. That is to say, when cue get go attempts to load type information for the cuelang.org/go/cmd/cue/cmd/interfaces package, it will add a dependency to cuelang.org/go to the go.mod as required
Example 1 fails with Go 1.16. This is because in 1.16, by default build commands do not update go.{mod,sum} - they behave as if -mod=readonly was specified. See cmd/go: default to & improve -mod=readonly golang/go#40728

Example 2

Now consider:

exec go mod init foo.bar/intstr
exec cue mod init foo.bar/intstr
exec go mod vendor
exec cue get go k8s.io/apimachinery/pkg/util/intstr
cmp cue.mod/gen/k8s.io/apimachinery/pkg/util/intstr/intstr_go_gen.cue intstr_go_gen.cue.golden

-- repro.go --
package intstr
import (
        "k8s.io/apimachinery/pkg/util/intstr"
)
_ = intstr.IntOrString
-- intstr_go_gen.cue.golden --
// Code generated by cue get go. DO NOT EDIT.

//cue:generate cue get go k8s.io/apimachinery/pkg/util/intstr

package intstr

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: _

// Type represents the stored type of IntOrString.
#Type: int64 // #enumType

#enumType:
	#Int |
	#String

#Int:    #Type & 0
#String: #Type & 1

i.e. we add a go mod vendor step.

Example 2 works with Go <= 1.13X
Example 2 fails with Go >= 1.14, because 1.14 introduced the concept of honouring a vendor directory if it exists (cmd/go: automatically check and use vendored packages golang/go#33848 (comment)). Furthermore, a vendor is not modified/updated if a dependency is missing, unlike the pre Go 1.16 behaviour without vendor

Conclusion

The version of cuelang.org/go/cmd/cue/cmd/interfaces loaded should correspond to the version of cmd/cue being used, not a version loaded via a go.mod requirement on cuelang.org/go
As such, we should embed cuelang.org/go/cmd/cue/cmd/interfaces (@verdverm I will comment over in https://cue-review.googlesource.com/c/cue/+/8021 if you are still interested in pursuing that CL?). This change will include proper surfacing of errors of any packages loaded, including cuelang.org/go/cmd/cue/cmd/interfaces
In https://cue-review.googlesource.com/c/cue/+/8021 we should add a cue get go test that exercises the loading of types that follow these rules:

https://github.com/cuelang/cue/blob/f0c025c1a4cb7602e497445a58578212730c3698/cmd/cue/cmd/get_go.go#L99-L105

Given the failure of Example 1 with Go 1.16, we need to make some sort of change/fix. I think this also gives us the opportunity to decide whether we want cue get go to be a wrapper around go get or not. The "fix" for Go 1.16 will be to add a cmd/go get step as part of cue get go. However, I think then forces us to have cue get go mirror cmd/go get in terms of docs/behaviour. This feels a bit unnecessary to my mind. Perhaps better would be to not try and wrap cmd/go get at all, but instead simply fail if the package(s) passed to cue get go fail to load (which would require the user to do something via cmd/go {get,mod tidy}). If we go down this path (which seems to make sense) then I'm not sure that cue get is necessarily the right/best name - but it's not totally clear cut one way or the other

cueckoo · 2021-07-03T10:55:50Z

Original reply by @myitcv in cuelang/cue#621 (comment)

Noting an offline conversation with @mpvl. Next steps:

fix the loading of the cuelang.org/go/cmd/cue/cmd/interfaces package by embedding, via https://cue-review.googlesource.com/c/cue/+/8021 or equivalent. Add tests that exercise the various rules in https://github.com/cuelang/cue/blob/f0c025c1a4cb7602e497445a58578212730c3698/cmd/cue/cmd/get_go.go#L99-L105
decide which errors for loading cuelang.org/go/cmd/cue/cmd/interfaces or arguments to cue get go should be surfaced. If we are unable to resolve a package, that is a fatal error. However a package not type checking is arguably not fatal: cue get go could be considered a best-effort if code does not fully type check
change cue get go to error in case any package arguments it is passed cannot be loaded (note this does not include cuelang.org/go/cmd/cue/cmd/interfaces). This will mean the user has to ensure that a package argument to cue get go can be resolved via go list -mod=readonly pkgs...
update all docs to add additional go get steps as necessary (although it appears the Kubernetes example is already clear on this: https://github.com/cuelang/cue/blame/f0c025c1a4cb7602e497445a58578212730c3698/doc/tutorial/kubernetes/README.md#L1062)
deprecate cue get go in favour of cue import go (cmd/cue: rename 'get go' to 'import go' cuelang/cue#646) - this naming better reflect that cmd/cue is not doing any sort of dependency management a la go get. Must include updating all docs
in a later release, remove cue get go

cueckoo · 2021-07-03T10:55:53Z

Original reply by @myitcv in cuelang/cue#621 (comment)

One further aspect we will need to consider, that isn't purely a function of the changes in Go 1.16 but is exacerbated by it.

Consider the following:

# Initial run
exec go get k8s.io/api/core/v1@v0.20.2
exec cue get go k8s.io/api/core/v1
exec cue def -e '#Service' k8s.io/api/core/v1

# Tidy then re-run
exec go mod tidy
exec cue get go k8s.io/api/core/v1

-- go.mod --
module blah.com

-- blah.go --
package blah

-- cue.mod/module.cue --
module: "blah.com"

With Go < 1.16 this will "work" because, despite the go mod tidy, the go/packages.Load call in the subsequent cue get go will re-add the module providing k8s.io/api/core/v1 to the go.mod.

That is to say, the side effects of Go < 1.16 were hiding a bigger issue, namely that the dependencies used as input to cue get go are not captured anywhere.

There is a precedent however for keeping track of such dependencies: tools. The current best practice for keeping track of tool dependencies is as follows (per golang/go#25922 (comment)): create a build-ignored .go file that declares the dependencies as underscore-imports. Rewriting the example above:

# Initial run
exec go get k8s.io/api/core/v1@v0.20.2
exec cue get go k8s.io/api/core/v1
exec cue def -e '#Service' k8s.io/api/core/v1

# Tidy then re-run
exec go mod tidy
exec cue get go k8s.io/api/core/v1

-- go.mod --
module blah.com

-- blah.go --
package blah

-- cue.mod/module.cue --
module: "blah.com"

-- cuegetgo.go --
// +build cuegetgo

package cuegetgo

import (
	_ "k8s.io/api/core/v1"
)

This advice works for all Go versions, and should probably have been the advice from the beginning (otherwise you have an unstable go.{mod,sum} files).

This is a point to consider for the docs/Go 1.16 docs for the cue get go/cue import go command.

cueckoo · 2021-07-03T10:55:55Z

Original reply by @NicolasRouquette in cuelang/cue#621 (comment)

@myitcv Thanks for improving our earlier analysis; LGTM.

I also want to note that during my analysis, I found a potentially confounding problem.
See: https://cuelang.slack.com/archives/CR7BJ142F/p1609288836062200?thread_ts=1609225131.058200&cid=CR7BJ142F

It seems to me that there's at least one bug, calls to packages.Load(...) from "golang.org/x/tools/go/packages" currently only check for errors in the return.
However, it seems that the callers should also check whether the returned packages have errors according to the function's doc:
// Load loads and returns the Go packages named by the given patterns.
//
// Config specifies loading options;
// nil behaves the same as an empty Config.
//
// Load returns an error if any of the patterns was invalid
// as defined by the underlying build system.
// It may return an empty list of packages without an error,
// for instance for an empty expansion of a valid wildcard.
// Errors associated with a particular package are recorded in the
// corresponding Package's Errors list, and do not cause Load to
// return an error. Clients may need to handle such errors before
// proceeding with further analysis. The PrintErrors function is
// provided for convenient display of all errors.
func Load(cfg *Config, patterns ...string) ([]*Package, error) {

cueckoo · 2021-07-03T10:55:56Z

Original reply by @myitcv in cuelang/cue#621 (comment)

Thanks, @NicolasRouquette - yes, that point is covered in cuelang/cue#621 (comment):

decide which errors for loading cuelang.org/go/cmd/cue/cmd/interfaces or arguments to cue get go should be surfaced. If we are unable to resolve a package, that is a fatal error. However a package not type checking is arguably not fatal: cue get go could be considered a best-effort if code does not fully type check

With the loading of the embedded cuelang.org/go/cmd/cue/cmd/interfaces we should tolerate no errors whatsoever.

With the loading of the arguments to cue get go we should probably tolerate parse and type errors, but no others. I've made comments to this effect over in https://cue-review.googlesource.com/c/cue/+/8021 (we should continue that dialogue there if people disagree)

cueckoo · 2021-07-03T10:56:16Z

Original reply by @NicolasRouquette in cuelang/cue#621 (comment)

FYI: This bug is still present in cue-0.3.0-beta3.

cueckoo · 2021-07-03T10:56:18Z

Original reply by @myitcv in cuelang/cue#621 (comment)

That's expected - the CL that fixes this is in progress. This issue will close automatically when the CL is merged.

cueckoo added get go issues related to cue get go NeedsFix labels Jul 3, 2021

cueckoo added this to the v0.3.0-evaluator-rewrite milestone Jul 3, 2021

cueckoo closed this as completed Jul 3, 2021

cueckoo mentioned this issue Jul 3, 2021

cmd/cue: improve error messages for get go #701

Open

cueckoo mentioned this issue Jul 3, 2021

Document usage of intstr.IntOrString in the k8s tutorial #148

Closed

cueckoo mentioned this issue Jul 3, 2021

cue get k8s.io/api/core/v1 creates an empty directory #95

Closed

This was referenced Jul 3, 2021

cmd/cue: get go panics #668

Closed

Proposal: package management #851

Closed

cue get go does not work consistently cuelang/cue#621

Closed

myitcv mentioned this issue Jul 15, 2021

cue get go panics when extracting definition from envoyproxy/go-control-plane #1097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cue get go does not work consistently #621

cue get go does not work consistently #621

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cue get go does not work consistently #621

cue get go does not work consistently #621

Comments

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

Example 1

Example 2

Conclusion

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021

cueckoo commented Jul 3, 2021