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.

Sign up for GitHub

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

Jump to bottom

glide: use jessevdk/go-flags for consistency. #833

Merged
merged 1 commit into from
Aug 31, 2017
Merged

glide: use jessevdk/go-flags for consistency. #833

merged 1 commit into from
Aug 31, 2017

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Aug 24, 2017

Fixes #804

davecgh
davecgh requested changes Aug 28, 2017
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't how glide works. The YAML file is a recipe that specifies the locations of the dependencies, perhaps with more generic semantic versions, while the lock file is the one that pins those specific versions.

glide.yaml Outdated
- package: github.com/btcsuite/go-socks
version: 4720035b7bfd2a9bb130b1c184f8bbe41b6f0d0f
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specific commit versions should not be in the yaml. That is what the lock file is for.

glide.yaml Outdated
subpackages:
- socks
- package: github.com/btcsuite/goleveldb
version: 7834afc9e8cd15233b6c3d97e12674a31ca24602
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

glide.yaml Outdated
@@ -19,26 +23,34 @@ import:
- leveldb/opt
- leveldb/util
- package: github.com/btcsuite/websocket
version: 31079b6807923eb23992c421b114992b95131b55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

glide.yaml Outdated
- package: github.com/btcsuite/winsvc
version: f8fb11f83f7e860e3769a08e6811d1b399a43722
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

glide.yaml Outdated
subpackages:
- eventlog
- mgr
- svc
- package: github.com/davecgh/go-spew
version: adab96458c51a58dc1783b3335dcce5461522e75
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

glide.yaml Outdated
subpackages:
- spew
- package: github.com/dchest/blake256
version: dee3fe6eb0e98dc774a94fc231f85baf7c29d360
- package: github.com/decred/bitset
version: 484b833245d5f9046e2893a6bd2e54b1df3a53a4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

glide.yaml Outdated
- package: github.com/decred/dcrrpcclient
version: abc326aeb97b299c7eb6b13b81353fc375e9b95d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

glide.yaml Outdated
- package: github.com/decred/dcrutil
version: 4ce0f41cf3f957fcdf3f0de049602ac834449b9c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

glide.yaml Outdated
subpackages:
- bloom
- package: golang.org/x/crypto
version: 6914964337150723782436d56b3f21610a74ce7b
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

glide.yaml Outdated
subpackages:
- ripemd160
- ssh/terminal
- package: github.com/jrick/logrotate
version: a93b200c26cbae3bb09dd0dc2c7c7fe1468a034a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

@dnldd
Copy link
Member Author

dnldd commented Aug 28, 2017

@davecgh the repos for the highlighted lines for the dependencies above don't have numbered versions. They either have outdated releases (in comparison to the versions specified in the .lock file) or none at all. The only way to specify dep versions is via commits (as done above), without this there isn't any way to specify versions in the .yaml file.

Because the dep versions aren't set in dcrd's current .yaml file, running glide up updates dependencies to their respective repo branch tips (which is undesirable). Please reconsider.

@davecgh
Copy link
Member

davecgh commented Aug 28, 2017 •
edited
Loading

Running glide up is supposed to update the lock file to latest versions. It's up to the maintainers (aka us) to choose whether or not we want to update the project dependencies to those versions or not. This change would completely break the intended purpose of glide up.

@jrick Would you mind chiming in here? I know you use deps extensively in dcrwallet.

@jrick
Copy link
Member

jrick commented Aug 28, 2017

I'm going to link to the cargo documentation here because i think it explains it better than glide, but glide follows the exact same methodology.

http://doc.crates.io/guide.html#cargotoml-vs-cargolock

We should not be specifying exact commits in glide.yaml except under very particular circumstances.

@dnldd
Copy link
Member Author

dnldd commented Aug 28, 2017

@jrick thanks for that link. @davecgh reverting.

@dnldd dnldd force-pushed the dependencyversions branch from 3da0d8c to b8e99a1 Compare August 28, 2017 20:08
@dnldd dnldd changed the title glide: explicitly set dep versions. glide: use github.com/jessevdk/go-flags. Aug 28, 2017
@dnldd dnldd force-pushed the dependencyversions branch from b8e99a1 to a841fad Compare August 28, 2017 20:14
@dnldd dnldd changed the title glide: use github.com/jessevdk/go-flags. glide: use jessevdk/go-flags for consistency. Aug 28, 2017
@dnldd
Copy link
Member Author

dnldd commented Aug 28, 2017

@davecgh updated the PR, should be good now.

jrick
jrick requested changes Aug 28, 2017
View reviewed changes
glide.yaml Outdated
subpackages:
- edwards25519
- package: github.com/btcsuite/btclog
- package: github.com/btcsuite/go-flags
- package: github.com/jessevdk/go-flags
version: ^1.3.0
- package: github.com/btcsuite/go-socks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the btcsuite fork of go-flags should be removed since we are switching to upstream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented on wrong file, it might be a transient dep in case we need to figure out what imported package is requiring it.

@dnldd dnldd force-pushed the dependencyversions branch 3 times, most recently from b7b3881 to b3a3d67 Compare August 28, 2017 21:57
@davecgh
Copy link
Member

davecgh commented Aug 30, 2017 •
edited
Loading

Needs a rebase to resolve merge conflicts, but otherwise this looks ready.

@dnldd dnldd force-pushed the dependencyversions branch 4 times, most recently from 99a7060 to 9bbb6bd Compare August 30, 2017 23:21
@dnldd dnldd force-pushed the dependencyversions branch from 9bbb6bd to f90c43e Compare August 30, 2017 23:48
@davecgh davecgh merged commit 539fb84 into decred:master Aug 31, 2017
@dnldd dnldd deleted the dependencyversions branch August 31, 2017 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davecgh davecgh davecgh approved these changes

@jrick jrick jrick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dnldd @davecgh @jrick