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.

Sign up for GitHub

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

Jump to bottom

Consider cutting down heavy dependencies #615

Open
misha-ridge opened this issue Dec 26, 2022 · 7 comments
Open

Consider cutting down heavy dependencies #615

misha-ridge opened this issue Dec 26, 2022 · 7 comments
Labels
dependencies Pull requests that update a dependency file

Comments

@misha-ridge
Copy link

misha-ridge commented Dec 26, 2022 •
edited

x/oauth2 has some churn due to security issues and changes in dependencies.

However the dependencies of x/oauth2 are not so set in stone:

  • Heavy cloud.google.com/go is required only for fetching GCE metadata. However the package for fetching metadata is pretty self-contained and can be pulled in.
  • Heavy google.golang.org/appengine is required only for Gen1 AppEngine. I haven't investigated how much code is actually used.
    After removal of these two dependencies, the rest is trivial:
  • x/net is used in one place to save 5 lines of code. (removed in e07593a)
  • google/go-cmp is used in one test to save 11 lines of code.

I currently maintain a dependency-less version of x/oauth2 at https://github.com/ridge/oauth2. Here are the patches: master...ridge:oauth2:master

The only feature loss is AppEngine Gen1 environment: for expediency I cut it out instead of figuring out if it can be retained.
@fserb
Copy link

fserb commented Feb 15, 2023

A thousand times this, please.

@lfittl
Copy link

lfittl commented Feb 25, 2023

I currently maintain a dependency-less version of x/oauth2 at https://github.com/ridge/auth2. Here are the patches: master...ridge:oauth2:master

FWIW, I got a 404 on that link - looks like the correct link is https://github.com/ridge/oauth2 (with a leading o)

(I just ran into this exact problem thanks to go mod vendor not supporting build constraints, and so even though the appengine file is marked "+build appengine" that doesn't prevent vendoring from pulling it in)

lfittl added a commit to pganalyze/collector that referenced this issue Feb 25, 2023
@lfittl
Update from github.com/golang/protobuf to google.golang.org/protobuf
88118fb 
The old module, github.com/golang/protobuf, has been deprecated a while,
and the newer API has a slightly improved interface for the timestamp
type as well, avoiding a potential error that needs to be handled (that
we've traditionally ignored anyway in our own code).

Note there is a remaining caller of the old module in Google's appengine
module, which gets vendored in unnecessarily via x/oauth2 (golang/oauth2#615).

In passing, update the protoc-gen-go tool as well to the version hosted
in the new repository.

Depends on pganalyze/collector-snapshot#30
@lfittl lfittl mentioned this issue Feb 25, 2023
Merged
@misha-ridge
Copy link
Author

@lfittl Thanks, updated.

lfittl added a commit to pganalyze/collector that referenced this issue Feb 28, 2023
@lfittl
Update from github.com/golang/protobuf to google.golang.org/protobuf
43ca13b 
The old module, github.com/golang/protobuf, has been deprecated a while,
and the newer API has a slightly improved interface for the timestamp
type as well, avoiding a potential error that needs to be handled (that
we've traditionally ignored anyway in our own code).

Note there is a remaining caller of the old module in Google's appengine
module, which gets vendored in unnecessarily via x/oauth2 (golang/oauth2#615).

In passing, update the protoc-gen-go tool as well to the version hosted
in the new repository.

Depends on pganalyze/collector-snapshot#30
lfittl added a commit to pganalyze/collector that referenced this issue Feb 28, 2023
@lfittl
Update from github.com/golang/protobuf to google.golang.org/protobuf (#…
08cb680 
…382)

The old module, github.com/golang/protobuf, has been deprecated a while,
and the newer API has a slightly improved interface for the timestamp
type as well, avoiding a potential error that needs to be handled (that
we've traditionally ignored anyway in our own code).

Note there is a remaining caller of the old module in Google's appengine
module, which gets vendored in unnecessarily via x/oauth2 (golang/oauth2#615).

In passing, update the protoc-gen-go tool as well to the version hosted
in the new repository.

Depends on pganalyze/collector-snapshot#30
@hickford hickford added the dependencies Pull requests that update a dependency file label Mar 10, 2023
@therve therve mentioned this issue May 30, 2023
Closed
@hickford
Copy link
Contributor

hickford commented Sep 7, 2023 •
edited

Great idea!

A little copying is better than a little dependency

https://go-proverbs.github.io/

@trakhimenok trakhimenok mentioned this issue Oct 6, 2023
Closed
@sg0hsmt
Copy link

sg0hsmt commented Mar 9, 2024 •
edited

google.golang.org/appengine is imported from two files, both of which have the appengine build tag set.

However, in appengine Go 1.11, the appengine build tag is no longer used, so these files are now unused and the dependencies can be removed.

refs: #334

@liggitt
Copy link
Contributor

liggitt commented Mar 11, 2024

This repo is actually no longer even buildable / testable with go1.11 (it requires go1.17+):

$ go version
go version go1.11.13 linux/amd64

$ go build ./...
# golang.org/x/oauth2/internal
internal/token.go:140:10: lc.v.CompareAndSwap undefined (type atomic.Value has no field or method CompareAndSwap)

$ go test ./...
# github.com/google/go-cmp/cmp
../../../github.com/google/go-cmp/cmp/report_compare.go:249:29: r.Value.ValueX.IsZero undefined (type reflect.Value has no field or method IsZero)
../../../github.com/google/go-cmp/cmp/report_compare.go:249:56: r.Value.ValueY.IsZero undefined (type reflect.Value has no field or method IsZero)
../../../github.com/google/go-cmp/cmp/report_compare.go:251:29: r.Value.ValueX.IsZero undefined (type reflect.Value has no field or method IsZero)
../../../github.com/google/go-cmp/cmp/report_compare.go:253:29: r.Value.ValueY.IsZero undefined (type reflect.Value has no field or method IsZero)
../../../github.com/google/go-cmp/cmp/report_reflect.go:194:9: vv.IsZero undefined (type reflect.Value has no field or method IsZero)
# golang.org/x/oauth2/internal
internal/token.go:140:10: lc.v.CompareAndSwap undefined (type atomic.Value has no field or method CompareAndSwap)
FAIL	golang.org/x/oauth2 [build failed]
FAIL	golang.org/x/oauth2/authhandler [build failed]
FAIL	golang.org/x/oauth2/clientcredentials [build failed]
FAIL	golang.org/x/oauth2/endpoints [build failed]
FAIL	golang.org/x/oauth2/google [build failed]
FAIL	golang.org/x/oauth2/google/downscope [build failed]
FAIL	golang.org/x/oauth2/google/externalaccount [build failed]
FAIL	golang.org/x/oauth2/google/internal/externalaccountauthorizeduser [build failed]
FAIL	golang.org/x/oauth2/google/internal/stsexchange [build failed]
# golang.org/x/oauth2/internal [golang.org/x/oauth2/internal.test]
internal/token.go:140:10: lc.v.CompareAndSwap undefined (type atomic.Value has no field or method CompareAndSwap)
FAIL	golang.org/x/oauth2/internal [build failed]
FAIL	golang.org/x/oauth2/jira [build failed]
ok  	golang.org/x/oauth2/jws	(cached)
FAIL	golang.org/x/oauth2/jwt [build failed]

I opened https://go-review.googlesource.com/c/oauth2/+/570595 to drop the appengine gen 1 code which is no longer possible to exercise on those Go versions.

@liggitt
Copy link
Contributor

liggitt commented Mar 11, 2024

Also opened googleapis/google-cloud-go#9545 which will make the cloud.google.com/go/compute/metadata dependency a zero-dep module.

gopherbot pushed a commit that referenced this issue Mar 11, 2024
@liggitt @hickford
appengine: drop obsolete code for AppEngine envs <=Go 1.11
3a6776a 
This library no longer builds on Go versions prior to Go 1.17,
so no longer needs to support compilation specific to AppEngine
environments on Go versions prior to Go 1.11

Related to #615

Change-Id: Ia9579ea2091cb86ee96065affb920370c4ba33ea
Reviewed-on: https://go-review.googlesource.com/c/oauth2/+/570595
Reviewed-by: Matt Hickford <matt.hickford@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cody Oss <codyoss@google.com>
Run-TryBot: Matt Hickford <matt.hickford@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@sagikazarmark sagikazarmark mentioned this issue Apr 17, 2024
Closed
gopherbot pushed a commit that referenced this issue Apr 17, 2024
@sagikazarmark @codyoss
go.mod: update cloud.google.com/go/compute/metadata dependency
4b7f0bd 
Related #615
Related googleapis/google-cloud-go#9545

Change-Id: I762dc53c61204ec5103336dab6358b9b4d1337d4
GitHub-Last-Rev: 439c393
GitHub-Pull-Request: #719
Reviewed-on: https://go-review.googlesource.com/c/oauth2/+/579495
Reviewed-by: Cody Oss <codyoss@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cody Oss <codyoss@google.com>
Reviewed-by: Jordan Liggitt <liggitt@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fserb @lfittl @hickford @liggitt @sg0hsmt @misha-ridge