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

x/vgo: replaces of dependencies are not ahered even though there's no reason not to #25391

Closed
JelteF opened this issue May 14, 2018 · 8 comments

Comments

@JelteF
Copy link
Contributor

commented May 14, 2018

What version of Go are you using (go version)?

go version go1.10.2 linux/amd64 vgo:2018-02-20.1

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Linux

What did you do?

I've created some repos that are dependent on each other to reproduce the problem:

  • github.com/JelteF/vgo-test-a
  • github.com/JelteF/vgo-test-b
  • github.com/JelteF/vgo-test-c
  • github.com/JelteF/vgo-test-c-fork

The dependencies are as follows:

  • A requires B
  • B requires C
  • B has a replace for C to C-fork (because C-fork has a fix for C)

Then I run the following in A:

vgo run main.go

What did you expect to see?

That it prints out:

correct

Which is what it would do if it uses the patched forked version of C

What did you see instead?

It prints out:

wrong

Which means it's using the unpatched version of C

Reasoning

A doesn't depend on a specific version of C. The only dependency requirements given for C are the ones given by B, so there's no reason not to fully go along with it.

Actual use case

I have this kind of situations happen when using the (non public) utils package that we have at my company. Sometimes one of it's dependencies need a fix or new feature and we want to use a fork temporarily while we wait for the fix to be merged. With dep this is easy right now by using the source attribute in the utils package Gopkg.toml and then update the utils package in all packages that use it. With vgo it seems to me that every package that depends on the utils packgae would need to add the replace to its own go.mod file. And after it is merged upstream we have to remove the replace everywhere again.

Some other related ideas

  1. A similar thing seems true to me for an excludedetermined by a package when running vgo get -u. It would be nice if it would try to exclude those packages, so not every library user has to find out that the excluded version is incompatible with
  2. If the MVS solver ignores a replace or exclude present in the go.mod of a library (e.g. because of conflicts), it would be nice if there was a warning stating that a replace/exclude that cannot be honoured. This way users have a much clearer path to fixing these kind of problems. I watched the GopherCon SG video, but the reasoning there doesn't apply when runnig vgo get -u. That still has the same problems as dep has, only there's no way for library authors to hint at a solution.
  3. For the replace it might be nice if the replace from a library is honoured even if the module that it replaces is also in the go.mod of the root module (given that the version there is <= than the replaced one). The root module author would still need a way to override this decission though, but that could for instance be achieved by also specifying a replace.

(feel free to create new issues for these other ideas, they seemed related enough that I think it's nice to keep discussion in one place for now)

EDIT: I'm not saying to give vgo a full SAT solver, I would just like some changes to MVS to make it find the "correct" version in ~90% percent of the cases and give useful output for the developer in the complicated 10%.

@gopherbot gopherbot added this to the vgo milestone May 14, 2018

@zeebo

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

Another way to solve your use case is to have your utils package copy and rename the dependency. Perhaps some tooling can be made available to help authors do that easily. For example, it could look at the go.mod, discover all of the replace directives, and automatically copy and rename them for you. The inverse operation would also be possible thanks to the metadata.

@JelteF

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

@zeebo That would indeed work in the simplified ABC example I created to show the problem. However in my real world utils version that's not the case. It would only work when the replaced dependency is never used directly by the main program. For most of the our real dependencies that is not true though. These dependencies are stuff for metrics, logging, tracing and grpc. The utils package contains some useful helpers and partial wrappers for those. But often the original libraries should still be used directly by the main program as well.

Edit: added the last couple of sentences, fat fingered the submit button

@kardianos

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

Can you explain why a utility dependency suddenly needed a fix and update? In this experience report can you give more specific details?

@JelteF

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

It's not just fixes, but also new features that you require upstream support.
I can give two examples where I was really happy that with dep I could easily use a fork of the library, that was then also used by the packages using our utils package. Both of these examples were with the logrus_sentry library:

  • When the whole sirupsen/Sirupsen debacle happened, logrus_sentry took a bit of time to be updated with the new casing. So we temporarily used a fork with the imports changed.
  • We've added a couple of new features to logrus_sentry that we wanted to use throughout our infrastructure. The pull requests were (like with any project) not merged the moment we created them. So we temporarily used our fork with the new features. (this one has happened twice)

I hope this makes the use case clearer for you.

@kardianos

This comment has been minimized.

Copy link
Contributor

commented May 20, 2018

@JelteF Can you give specific examples of this happening? How often do you use this feature in other dependency managers? I understand the "engineering design requirements" you are stating, I'm missing the experience report.

@JelteF

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2018

@kardianos your question reads like you missed my previous comment, that I posted after your first request for examples. If you did see it could you explain what is not clear about the examples I gave in that comment?

@kardianos

This comment has been minimized.

Copy link
Contributor

commented May 21, 2018

I saw it, but it wasn't really what I had in mind. I understand your example and your given requirements. I'm not in a position to make a call here.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

Sorry but no. This just creates a situation where B can be used only in programs that don't themselves also import C (because then there would be no good reason to apply the fork to those programs).

@rsc rsc closed this Jun 6, 2018

@golang golang locked and limited conversation to collaborators Jun 6, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.