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

Bugfix for change in Glide behavior #237

Merged
merged 1 commit into from
Dec 28, 2015
Merged

Conversation

akutz
Copy link
Member

@akutz akutz commented Dec 22, 2015

This patch fixes an issue (#221) where Glide no longer seems to handle transitive dependencies correctly unless the referencing dependency is ordered above another that may have a conflict.

In this instance the dependency upon github.com/akutz/goof was moved to the top of the glide.yaml file so that its dependency upon a fork of the logrus project would be honored when glide evaluated the transitive dependency chain.

@clintonskitson, FWIW, the reason you didn't see any speedup in builds with caching is how glide now operates. It apparently downloads everything by default, rendering the package cache somewhat irrelevant due to the fact that glide by default rebuilds it as it downloads dependencies anyway.

In the future we should eliminate transitive dependencies if we wish to avoid this so we can disable glide's recursive dependency feature.

@clintonskitson, I also made some small changes in the Makefile so that if you set MAKE_LOG_LEVEL to 2 via an environment variable, glide will output the log of what dependencies it downloads. This should allow us to confirm the correct logrus branch is being set to coincide with the testing I did locally.

@akutz akutz added the bugfix label Dec 22, 2015
@akutz akutz self-assigned this Dec 22, 2015
@akutz akutz added this to the 0.3.1 milestone Dec 22, 2015
This patch fixes an issue where Glide no longer seems to handle
transitive dependencies correctly unless the referencing dependency is
ordered above another that may have a conflict.

In this instance the dependency upon `github.com/akutz/goof` was moved
to the top of the `glide.yaml` file so that its dependency upon a fork
of the `logrus` project would be honored when glide evaluated the
transitive dependency chain.
@akutz
Copy link
Member Author

akutz commented Dec 22, 2015

This PR's build shows the logrus branch being set appropriately.

@mattfarina
Copy link

Thanks for pinging me into this (though the other issue).

Handling transitive dependency versions is hard in Go/Glide right now. If everyone was using SemVer ranges you could figure things out. Unfortunately, most of the time a commit id is used and that's hard to sort out. Right now we are doing the case where the first version wins and we provide a warning when another version comes along. Then in the glide.yaml file you can use ordering or listing a lower level dependency to get the version you want. It's not ideal. I'm open better suggestions.

I'm curious why you're putting the glide.lock file in .gitignore. Do you want to follow the tip of the branches for development?

A tip, while I'm here... you don't need to typically specify the vcs property in the glide.yaml. If Glide can detect the VCS it will do that work for you. In the case of packages on GitHub or of repos urls for GitHub Glide can automatically detect Git.

If you have suggestions for Glide we're open to listening. Hope this all goes well.

@akutz
Copy link
Member Author

akutz commented Dec 22, 2015

Hi @mattfarina,

We're ignoring the new lock file because I'm using a Maven approach to versioning dependencies. During active development we're depending on the tip of things, only locking down prior to release. This is what we did for the last release, and what we'll do this time. It worked well with glide.yaml. However, with glide.lock it appears that you want to lock dependencies every time, and this isn't something we want. I'm ignoring the file simply so it's not accidentally committed by some other developer. It will be created and used on the CI system per your docs of course, but it won't be a part of the repo and influencing builds unless we specifically lock a dependency via glide.yaml.

As for the ordering, I understand. The dependency resolution engine in Maven's reactor is a rather large piece of work and a majority of the value of Maven. It's no small feat to duplicate handling transitive conflicts and what not.

@clintkitson
Copy link
Member

Been testing this. Apparently I had a gremlin in my install where somehow my /go/src/github.com/emccode/rexray path got symbolically linked to another place I had downloaded the RR package to. I believe this was causing the vendoring to not kick in. Will finally have clean results here shortly.

@clintkitson
Copy link
Member

Finally, LGTM.

@mattfarina
Copy link

@akutz Thanks for the feedback. The way you're ignoring the lockfile for now is the right pattern for that situation. I've done the same thing in other languages where the tooling a similar lockfile pattern.

@clintkitson
Copy link
Member

Tested this on my own repo and travis integration to bintray. LGTM

https://bintray.com/clintonskitson/rexray/staged/0.3.1-rc3/view#files/staged/0.3.1-rc3

akutz added a commit that referenced this pull request Dec 28, 2015
Bugfix for change in Glide behavior
@akutz akutz merged commit f302968 into rexray:master Dec 28, 2015
akutz added a commit to akutz/rexray that referenced this pull request Jul 24, 2017
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.

None yet

3 participants