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

cue: Go API downloads more than necessary for simple use cases #1872

Closed
mvdan opened this issue Aug 23, 2022 · 4 comments
Closed

cue: Go API downloads more than necessary for simple use cases #1872

mvdan opened this issue Aug 23, 2022 · 4 comments
Assignees

Comments

@mvdan
Copy link
Member

mvdan commented Aug 23, 2022

Right now, a simple example like https://pkg.go.dev/cuelang.org/[email protected]/cue#example-Context downloads quite a lot of modules:

go: finding module for package cuelang.org/go/cue
go: finding module for package cuelang.org/go/cue/cuecontext
go: downloading cuelang.org/go v0.4.3
go: downloading github.com/cockroachdb/apd/v2 v2.0.1
go: downloading github.com/cockroachdb/apd v1.1.0
go: downloading github.com/mpvl/unique v0.0.0-20150818121801-cbe035fff7de
go: downloading golang.org/x/text v0.3.7
go: downloading golang.org/x/net v0.0.0-20200226121028-0de0cce0169b
go: downloading github.com/google/uuid v1.2.0
go: downloading github.com/pkg/errors v0.8.1
go: downloading gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b

Some of these look unnecessary, like pkg/errors and apd v1. We should trim these down.

@mvdan mvdan self-assigned this Aug 23, 2022
@mvdan
Copy link
Member Author

mvdan commented Aug 23, 2022

We can drop pkg/errors once cockroachdb/apd#123 is done.

cueckoo pushed a commit that referenced this issue Aug 23, 2022
apd/v2 version v2.0.1 had its v1 module as a test dependency.
That was fixed in v2.0.2, so pull it to trim our graph.

For #1872.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: I771d542f90a011040941fccdb8c1e65ccf25586e
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/542598
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Paul Jolly <[email protected]>
cueckoo pushed a commit that referenced this issue Aug 23, 2022
go-cmp, x/tools, and x/mod were all pulling it indirectly.
All of them removed it at some point in the last year.
Update them so we can drop x/xerrors.

No functional change, but our set of dependencies gets slightly lighter.

Updates #1872.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: Ie19eb3ea19467faca5313dfb038e14da641a8a4f
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/542609
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Roger Peppe <[email protected]>
cueckoo pushed a commit that referenced this issue Aug 23, 2022
This is more accorate than the generic ".txt" extension.
The latest testscript version supports this switch,
and in fact it now prefers the explicit extension.

The update drops errgo.v2 from the module graph, which helps #1872.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: I2fc1a108af6cd4544a00d0ffc3f334b5bbd6c720
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/542611
Reviewed-by: Roger Peppe <[email protected]>
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
@mvdan
Copy link
Member Author

mvdan commented Sep 12, 2022

Upstream apd fixed their reliance on pkg/errors, though they did that in v3, while we are in v2. @mpvl @myitcv would it be OK to upgrade to apd/v3 now? Per https://github.com/cockroachdb/apd/releases/tag/v3.0.0 there's only one breaking change, so it shouldn't be a hard upgrade. We probably also don't want to stay on v2 for too long, as it hasn't seen an update in over two years now.

@mvdan
Copy link
Member Author

mvdan commented Sep 13, 2022

I gave it a quick go, and it mostly works, though a few tests are unhappy due to different resulting precision. For example, in master with a bit of debug logging to see the apd.Decimal being marshaled:

$ go test -v -run TestMarshalJSON/22 ./cue
=== RUN   TestMarshalJSON
=== RUN   TestMarshalJSON/22/a:_1.0/1
apd.Decimal{Form:0, Negative:false, Exponent:-1, Coeff:big.Int{neg:false, abs:big.nat{0xa}}}
"1.0"
--- PASS: TestMarshalJSON (0.00s)
    --- PASS: TestMarshalJSON/22/a:_1.0/1 (0.00s)
PASS
ok  	cuelang.org/go/cue	0.005s

and now in my branch:

$ go test -v -run TestMarshalJSON/22 ./cue
=== RUN   TestMarshalJSON
=== RUN   TestMarshalJSON/22/a:_1.0/1
apd.Decimal{Form:0, Negative:false, Exponent:-23, Coeff:apd.BigInt{_inner:(*big.Int)(nil), _inline:[2]big.Word{0x2c7e14af6800000, 0x152d}}}
"1.00000000000000000000000"
    types_test.go:2721: 
         got {"a":1.00000000000000000000000};
        want {"a":1.0}
--- FAIL: TestMarshalJSON (0.00s)
    --- FAIL: TestMarshalJSON/22/a:_1.0/1 (0.00s)
FAIL
FAIL	cuelang.org/go/cue	0.006s

This seems like a change that we don't want, because we add verbosity for no reason. However I can't tell if it's a bug on our side (perhaps relying on undocumented behavior), or an intentional change in behavior in upstream's v3, which they are allowed to do per semver. I could not find anything in their release changelogs.

Worth noting that I think the new Exponent:-23 comes from CUE using an apd precision of 24.

I think upstream changed this in cockroachdb/apd#115, as it removes calls to Reduce from funcs like Quo, which is about removing trailing zeros. And this test in particular performs a division.

@mvdan
Copy link
Member Author

mvdan commented Sep 13, 2022

@myitcv myitcv added the zGarden label Jun 13, 2023
@mvdan mvdan removed the zGarden label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants