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

Go module support #5849

Closed
wants to merge 8 commits into from
Closed

Conversation

anacrolix
Copy link
Contributor

Per #5848 .

@anacrolix anacrolix requested a review from Kubuxu as a code owner December 17, 2018 04:00
License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
@lanzafame
Copy link
Contributor

@anacrolix See #5435 for moving to a gx unwritten state, which is essentially the same as moving to go modules just minus a go.mod and go.sum.

License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
License: MIT
Signed-off-by: Matt Joiner <anacrolix@gmail.com>
@Stebalien
Copy link
Member

This will need quite a bit of discussion before we consider it.

@Stebalien Stebalien closed this Dec 17, 2018
@anacrolix
Copy link
Contributor Author

Where would such a discussion occur?

@Stebalien
Copy link
Member

We generally like to discuss large changes in issues first. Both in your issue (to discuss adopting go modules) and #5850 (where I've brought up simply switching to them entirely).

@anacrolix anacrolix mentioned this pull request Dec 17, 2018
@mhchia mhchia mentioned this pull request Dec 23, 2018
@anacrolix
Copy link
Contributor Author

Can this be reopened?

@Stebalien
Copy link
Member

No, it cannot. See: #5850 (comment)

@anacrolix
Copy link
Contributor Author

I don't follow, the comment does not make sense.

@Stebalien
Copy link
Member

Stebalien commented Jan 2, 2019

The issue is that semver/go-dep go modules exist for a reason. If we switch over to go-mod now (without full go-dep mod support), we won't get any dependency version checking. That means we can easily end up in the following situation:

  1. Make a breaking change in a transitive dependency (D) of go-ipfs.
  2. Fix all packages (I) directly depending on D.
  3. Fix a bug in package D.
  4. In go-ipfs, update D without updating I (to pull in the bugfix).

gx forces exact version matching so isn't a problem. If we update package D, we'd have to use the latest versions of the packages in I.

go-dep mod fixes this problem using semver and version constraints.

@anacrolix
Copy link
Contributor Author

anacrolix commented Jan 2, 2019

Can you link to go-dep? I'm not sure what to what you refer. I don't see how the situation you describe is a problem. One would not update D for just for the sake of it (particularly if D is not a direct dependency of go-ipfs), and I's and go-ipfs's tests would fail if one did and it was a problem in either of them. Furthermore, if the change in D was truly breaking, the good author of that package would create a new major version preventing irresponsible updates downstream.

@Stebalien
Copy link
Member

Can you link to go-dep?

Sorry, too many package managers. I meant go-mod (the go module system).

Furthermore, if the change in D was truly breaking, the good author of that package would create a new major version preventing irresponsible updates downstream.

This ^^. If we use this PR as-is, we don't get the ability to specify version constraints like this.

Really, we don't need a breaking change to run into this issue. All we need to do is:

  1. Try to make a change in some package I.
  2. Realize there's a bug in package D (or a missing feature).
  3. Fix the bug/feature in package D.
  4. Update the minimum version of D in I.
  5. Make the desired change to I.
  6. Update package I in go-ipfs.

With full go modules support in packages D and I, updating the minimum version of package I in go-ipfs will automatically update the minimum version of D. As it stands, we could end up updating D without updating I.


Basically, we're already using a package manager with basic version constraint checking. Merging this PR without fully switching over all of our other packages to use go-mod would be a pretty big step backwards.

@anacrolix
Copy link
Contributor Author

As it is, people not being able to build without using a standard tool is the biggest blocker. This PR addresses that. Updates to transitive dependencies are still visible in updates downstream, they're listed as // indirect in the go.mod and should be present in commit diffs.

I find that MVS puts all the power in the hands of the most downstream project, which should be able to make the important decisions about dependencies. Existing popular package constraint systems allow middle packages to meddle and alter builds without agreement from the downstream projects. Maybe gx doesn't do that, but critically go.mod provides the ability to review and audit all changes project by project and issue immutable corrections if errors are made.

@Stebalien
Copy link
Member

Updates to transitive dependencies are still visible in updates downstream, they're listed as // indirect in the go.mod and should be present in commit diffs.

That only applies to dependencies that have go.mod files. My point was that, as-is, our packages don't have them (and most aren't even using semver).

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 this pull request may close these issues.

3 participants