Revert vgo support #563
Merged
Revert vgo support #563
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reverts #562 where we added support for vgo. I'd tested vgo's behavior on a simple synthetic versioned repository before merging #562, but apparently it wasn't enough because I'm unable to correctly add stripe-go with vgo even with the addition of a `go.mod` and corresponding tag. Here's the resolution error when trying to build a test project: ``` $ vgo build vgo: resolving import "github.com/stripe/stripe-go/v30" vgo: finding github.com/stripe/stripe-go/v30 (latest) vgo: adding github.com/stripe/stripe-go/v30 v30.8.1 vgo: finding github.com/stripe/stripe-go/v30 v30.8.1 vgo: downloading github.com/stripe/stripe-go/v30 v30.8.1 vgo: resolving import "github.com/stripe/stripe-go/form" vgo: finding github.com/stripe/stripe-go (latest) vgo: adding github.com/stripe/stripe-go v1.0.3 vgo: import "github.com/brandur/stripe-go-vgo" -> import "github.com/stripe/stripe-go/v30": found in both github.com/stripe/stripe-go v1.0.3 and github.com/stripe/stripe-go/v30 v30.8.1 ``` I think what's happening is that it correctly sees the `v30.*` tag and resolves that, but then it starts to descend into subpackages, and eventually finds one that references the top-level package (almost all of them do, but take `account/` as an example). The import doesn't use the `stripe/stripe-go/v30` suffix, and therefore vgo tries to resolve a different package version (once again falling back to `v1.0.3`), and produces a conflict. I think the fix is to make sure all imports end in `/v30`, but this is very problematic from two angles: 1. It breaks our compatibility with non-vgo installations. 2. It forces us to update the import path on dozens of subpackages every time we do a new major release, which is very bad. For now, I'm going to revert this. Hopefully some progress on vgo in the future makes this a little easier.
brandur-stripe
force-pushed
the
brandur-revert-vgo
branch
from
d08a9fa to
c3a8d01
Compare
Closed
|
Thanks Remi! |
brandur-stripe
pushed a commit
that referenced
this pull request
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.
nadaismail-stripe
pushed a commit
that referenced
this pull request
Oct 18, 2024
* Improve deletion script, add refresh script * bump sfdx version * slight formatting improvement
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #562 where we added support for vgo.
I'd tested vgo's behavior on a simple synthetic versioned repository
before merging #562, but apparently it wasn't enough because I'm unable
to correctly add stripe-go with vgo even with the addition of a
go.modand corresponding tag.
Here's the resolution error when trying to build a test project:
I think what's happening is that it correctly sees the
v30.*tag and resolvesthat, but then it starts to descend into subpackages, and eventually finds one
that references the top-level package (almost all of them do, but take
account/as an example). The import doesn't use thestripe/stripe-go/v30suffix, and therefore vgo tries to resolve a different package version (once
again falling back to
v1.0.3), and produces a conflict.I think the fix is to make sure all imports end in
/v30, but this is veryproblematic from two angles:
we do a new major release, which is very bad.
For now, I'm going to revert this. Hopefully some progress on vgo in the future
makes this a little easier.