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

Add go.mod for vgo #559

Closed
egtann opened this issue May 22, 2018 · 15 comments · Fixed by #562
Closed

Add go.mod for vgo #559

egtann opened this issue May 22, 2018 · 15 comments · Fixed by #562

Comments

@egtann
Copy link

egtann commented May 22, 2018

The vgo proposal was just accepted: golang/go#24301 (comment), and all signs indicate that this is the direction go get will head.

When using vgo to import the latest version of this library today, it pulls in v1.0.3, which is the highest version of v1. Specifying a go.mod file and using its versioning scheme would allow users of vgo (and soon go get) to pull in the correct version by default, which as of now is v30.

https://research.swtch.com/vgo-module

@brandur-stripe
Copy link
Contributor

Interesting. Yeah, it couldn't hurt to make sure we have a go.mod. I'll need to tweak our release system to make sure that it updates it correctly, but should be able to add one a little later today.

@brandur-stripe
Copy link
Contributor

So the only thing that's a little unfortunate about this is that it seems that the system mandates that new major versions be made by path, so our go.mod would look like this:

module "github.com/stripe/stripe-go/v30"

And that users should then use it like this from their *.go files:

"github.com/stripe/stripe-go/v30"

Which will cause vgo to produce something like this in mod.go:

require github.com/stripe/stripe-go/v30 v30.0.0

Failure to include the trailing v30 will throw you all the way back to v1.0.3 as you mentioned above. I guess it's not the end of the world, but this is a slight departure from how we've recommended people version in the past — we don't produce that many major versions anymore, but we do follow semver pretty aggressively, and will produce one for any backwards incompatible changes, so it's convenient just to point people to a path without a version in it. Did I miss something here?

@brandur-stripe
Copy link
Contributor

This also has the secondary problem of us having no way to give a single import recommendation to both vgo and non-vgo users.

We'd want Vgo users to use this:

import "github.com/stripe/stripe-go/v30"

So that they don't end up with v1-something, but if we ask non-Vgo users to do that, their build will fail because "v30" isn't a real path — only a symbolic one that's understood by Vgo.

Going to hold off on this until we have a better answer.

@egtann
Copy link
Author

egtann commented May 23, 2018

Looks like a better upgrade path is coming in 1.9.7 and 1.10.3:

https://github.com/golang/go/issues?utf8=%E2%9C%93&q=is%3Aissue+add+minimal+support+for+vgo+transition+backport

https://go-review.googlesource.com/c/go/+/109340

So the old go get and the new vgo behavior will be able to coexist in the same repo. The best path may be to wait for those point releases.

@brandur-stripe
Copy link
Contributor

@egtann Phew. Yes, I think that will do it for us. Thanks for digging it up.

brandur-stripe pushed a commit that referenced this issue May 24, 2018
Adds a `go.mod` file to the project for aspiring vgo support. As
described in #559, without such a file, vgo will default to the lowest
released major version available, which in our case is `v1.0.3`. The vgo
proposal seems to have been officially accepted, so having support for
it early is a bit of a nicety that we can offer.

The one caveat of this addition is that while vgo users should import
stripe-go like this:

    import (
        "github.com/stripe/stripe-go/v30"
    )

We can't actually recommend this `import` style anywhere in our core
documentation yet (although it's okay to put it in the `README.md` in a
vgo-specific section). This is because the `v30` suffix at the end of
the path isn't a real physical path, but rather just a symbolic one that
vgo knows how to parse and resolve as a tag. Trying to use this import
with convention `go get` will fail.

The good news is that this problem has been acknowledged by the core
team, and there will eventually be a pre-vgo `go get` that understands
how to do basic tag resolution [1]. At some point in the future after
this has become widely available, we can probably switch over to
recommending a single install invocation for vgo and pre-vgo users.

Fixes #559.

[1] https://go-review.googlesource.com/c/go/+/109340
brandur-stripe pushed a commit that referenced this issue May 24, 2018
Adds a `go.mod` file to the project for aspiring vgo support. As
described in #559, without such a file, vgo will default to the lowest
released major version available, which in our case is `v1.0.3`. The vgo
proposal seems to have been officially accepted, so having support for
it early is a bit of a nicety that we can offer.

The one caveat of this addition is that while vgo users should import
stripe-go like this:

    import (
        "github.com/stripe/stripe-go/v30"
    )

We can't actually recommend this `import` style anywhere in our core
documentation yet (although it's okay to put it in the `README.md` in a
vgo-specific section). This is because the `v30` suffix at the end of
the path isn't a real physical path, but rather just a symbolic one that
vgo knows how to parse and resolve as a tag. Trying to use this import
convention with `go get` will fail.

The good news is that this problem has been acknowledged by the core
team, and there will eventually be a pre-vgo `go get` that understands
how to do basic tag resolution [1]. At some point in the future after
this has become widely available, we can probably switch over to
recommending a single install invocation for vgo and pre-vgo users.

Fixes #559.

[1] https://go-review.googlesource.com/c/go/+/109340
@brandur-stripe
Copy link
Contributor

brandur-stripe commented May 24, 2018

And on second thought, it should be fine to just add a go.mod early, even if we won't recommend a vgo-compatible path as the primary way to install stripe-go for the time being. Added one in #562.

@brandur-stripe
Copy link
Contributor

brandur-stripe commented May 24, 2018

Alright, see #563 for details, but I ended up reverting vgo support because the relatively simple approach that I thought would work didn't.

I took a quick glance at the Go repository to see if there were any vgo issues where people were reporting similar problems, but there's enough noise there that I found it a little hard to tell. @egtann, if you the best way that we could report this as a real-world data point, or whether another improvement is in the pipeline to address it, let us know. Thanks!

@egtann
Copy link
Author

egtann commented May 24, 2018

You might want to post a message in https://groups.google.com/forum/#!forum/golang-nuts. I'm sure this is something Russ Cox and others spent a lot of time thinking about, and I'd also love a clear "here are the 5 steps to upgrade an existing v2+ library to use vgo" guide to send to the dependencies we use.

The upcoming point release should be the fix for all of this. Since the patch is being backported, it should give go 1.9+ the knowledge to ignore /vX in import paths iff the directory doesn't really exist and a go.mod defines that version. After that point release, you'll be able to update all import paths to use the new form appending /v30. People will be able to use go get (which will fetch HEAD, just as it does now and ignore the version info in import paths), or vgo (which will fetch the highest patch of the specified major version). In 1.11 I believe, go get's behavior will change to match vgo's behind a flag, and in 1.12, vgo's behavior is supposed to become the default.

It'll probably be a bit of a manual process to update those import paths at this point. Perhaps some go-specific tooling will be built to automate it (or already has been), but I use rpl to automate it in the interim.

@brandur-stripe
Copy link
Contributor

brandur-stripe commented May 25, 2018

Thanks. I'll try to get a message posted to that list at some point.

It'll probably be a bit of a manual process to update those import paths at this point. Perhaps some go-specific tooling will be built to automate it (or already has been), but I use rpl to automate it in the interim.

Yeah, we have some automated tooling already that could do the trick here, but I'm not crazy about just how many files would need to be changed every time we bumped the major version. Our setup is probably unusual, but you'd have to go in and bump dozens of subpackages, which seems pretty messy to me. It'd be much better if all packages within a module would automatically assume the same module version everywhere.

@egtann
Copy link
Author

egtann commented May 26, 2018

As one possible solution, you could avoid all of this by hoisting the form package contents to the top-level directory and fixing ./orderitem.go to no longer import the orderitem package (there's already a todo/cleanup comment there). Since the form package is required by most sub-packages, the root directory seems like an appropriate place for it even though it's not a domain type.

@brandur-stripe
Copy link
Contributor

brandur-stripe commented May 29, 2018

As one possible solution, you could avoid all of this by hoisting the form package contents to the top-level directory and fixing ./orderitem.go to no longer import the orderitem package (there's already a todo/cleanup comment there). Since the form package is required by most sub-packages, the root directory seems like an appropriate place for it even though it's not a domain type.

That order item problem is going to be fixed quite soon as part of #544.

Your interpretation here is that form being a package is what's causing the problem though? Admittedly I didn't do exhaustive testing, but it looked to me more of a problem caused by the subpackages for each resource (e.g., account) importing the root stripe package, and getting v1 instead of v30 because they don't have a version suffix. Something like:

stripe (module; v30)
    account/
        import "github.com/stripe/stripe-go" --> no suffix, so resolves v1 instead of v30

@tav
Copy link

tav commented Aug 26, 2018

Now that Go 1.11 has landed with support for Go modules, perhaps it's time to revisit this issue? Running go help modules should be helpful...

@tav
Copy link

tav commented Aug 26, 2018

Running go mod init && go mod tidy produces this go.mod file for me:

module github.com/stripe/stripe-go

require (
	github.com/davecgh/go-spew v1.1.1 // indirect
	github.com/pmezard/go-difflib v1.0.0 // indirect
	github.com/stretchr/testify v1.2.2
	golang.org/x/net v0.0.0-20180826012351-8a410e7b638d
	golang.org/x/text v0.3.0 // indirect
)

brandur-stripe pushed a commit that referenced this issue Sep 4, 2018
Adds a `go.mod` file to support the new experimental Go module system
which is likely to see full release pretty soon.

See #559 for original context. I originally tried this in #563, but ran
into trouble because Vgo wasn't smart enough to have subpackages in a
module know to use their parent's version, so we'd see problems where
it'd use stripe-go v46 (for example), go on to look at stripe-go/charge,
see that it depended on stripe-go, so it would re-ascend the tree and
default to stripe-go v1 instead of v46! See #563 for details, but it
didn't work at all and I ended up just reverting.

Since then it seems like some major improvements have been made and
subpackages can now resolve their parents version correctly. (I tried
this in a test project and although I'm not 100% sure that it was doing
t he right thing because `go.mod` is not yet in GitHub's master, it
looked like it was working).

Also, the system has improved to the point where the `/v46` path suffix
is no longer needed in `vgo.mod` which means that we don't even have to
updated our build scripts. That's great.

After this lands, I'll make sure to mess around it with a little more to
make sure everything looks like it's doing what expect.
@brandur-stripe
Copy link
Contributor

@tav Thanks for the info!

It looks like a lot of improvements and fixes have been made since the last time we tried this, so I took one more shot at it in #689, and things look promising.

@brandur-stripe
Copy link
Contributor

Alright, I'm going to call this done, but there's still some weirdness that I don't quite understand. If I start with a project that depends on stripe-go:

module github.com/stripe/stripe-go-mod-test

require github.com/stripe/stripe-go v47.0.0

But when the depends get resolved, it comes out the other side as a pseudo-version:

module github.com/stripe/stripe-go-mod-test

require github.com/stripe/stripe-go v0.0.0-20180904204438-999fc1da3647

I don't know why, but it does go to the right commit tag, and things are still working, so I'm going to close this out for the time being.

nadaismail-stripe pushed a commit that referenced this issue Oct 18, 2024
Bumps [sorbet-runtime](https://github.com/sorbet/sorbet) from 0.5.10139 to 0.5.10145.
- [Release notes](https://github.com/sorbet/sorbet/releases)
- [Commits](https://github.com/sorbet/sorbet/commits)

---
updated-dependencies:
- dependency-name: sorbet-runtime
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants