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

Migration from glide to dep #319

Merged
merged 4 commits into from
Aug 28, 2018
Merged

Migration from glide to dep #319

merged 4 commits into from
Aug 28, 2018

Conversation

soffokl
Copy link
Member

@soffokl soffokl commented Aug 24, 2018

No description provided.

@soffokl soffokl self-assigned this Aug 24, 2018
@soffokl soffokl requested review from tadovas and Waldz as code owners August 24, 2018 06:02

[[prune.project]]
name = "github.com/ethereum/go-ethereum"
unused-packages = false
Copy link
Contributor

Choose a reason for hiding this comment

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

New line expected

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

go-tests = true
unused-packages = true

[[prune.project]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Global unused-packages is a little evil I agree - so no we can specificy this flag per dependency? It would be a bit tricky to track if we need this flag for specific depedency or not - especially if it has transitive dependencies. and this flag is very important if package uses cgo - as any .c .h files will be pruned if this flag is true. That's a bit of consideration

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, now it can be used for per project configuration.
Regarding cases when we need to save unused-packages, I think, we should use it only when something does not work without it. It keeps a vendor directory much smaller.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright. That's a new feature I guess? As long as I remember it was global flag only

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure when it was introduced.
Found the only bug when it was broken: golang/dep#1561 )

@soffokl soffokl force-pushed the experiment/glide-to-dep branch from 76730d7 to 640b213 Compare August 24, 2018 06:16
@soffokl soffokl changed the title [WIP] Migration from glide to dep Migration from glide to dep Aug 24, 2018
tadovas
tadovas previously approved these changes Aug 24, 2018
Copy link
Contributor

@tadovas tadovas left a comment

Choose a reason for hiding this comment

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

LGTM

zolia
zolia previously approved these changes Aug 24, 2018
@@ -38,8 +34,8 @@ jobs:
- stage: dep-cache
name: "Vendor update"
script:
- source bin/travis_scripts/ensure_glide.sh $BUILD_TOOLS_PATH "v0.13.1"
- glide "-home" $GLIDE_HOME install
- curl https://raw.githubusercontent.com/golang/dep/master/install.sh | sh
Copy link
Member

Choose a reason for hiding this comment

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

What about go get .. way?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is strongly recommended that you use a released version.

And go get requires to get sources and build it, so it probably a bit slower than download just binary.

@@ -6,7 +6,7 @@
* **Step 1.** Get project dependencies
```bash
brew install go
brew install glide
brew install dep
Copy link
Member

Choose a reason for hiding this comment

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

We collect our development dependencies into node-builder image here bin/builder_docker/Dockerfile

Waldz
Waldz previously approved these changes Aug 24, 2018
@soffokl soffokl dismissed stale reviews from Waldz, zolia, and tadovas via 32065b7 August 27, 2018 03:48
@soffokl soffokl force-pushed the experiment/glide-to-dep branch 2 times, most recently from 32065b7 to 3a0251f Compare August 28, 2018 05:28
@soffokl soffokl force-pushed the experiment/glide-to-dep branch from 3a0251f to c031903 Compare August 28, 2018 05:30
@soffokl
Copy link
Member Author

soffokl commented Aug 28, 2018

@Waldz, @tadovas, @zolia could you please review it again, I have rebased it to master.

Copy link
Contributor

@tadovas tadovas left a comment

Choose a reason for hiding this comment

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

LGTM

@tadovas tadovas merged commit 806e10b into master Aug 28, 2018
@tadovas tadovas deleted the experiment/glide-to-dep branch August 28, 2018 07:13
@chompomonim chompomonim mentioned this pull request May 21, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants