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

Provide built-in Godep support #309

Merged
merged 2 commits into from Oct 19, 2014
Merged

Conversation

@zackkitzmiller
Copy link
Contributor

@zackkitzmiller zackkitzmiller commented Oct 18, 2014

As suggested by Josh Kalderimis here.

@BanzaiMan
Copy link
Member

@BanzaiMan BanzaiMan commented Oct 18, 2014

@meatballhat Could you take a look at this one when you get a chance?

@@ -33,6 +33,7 @@ def setup
# easier to find and our `git clone`'d libraries are found by the
# `go` commands.
set 'GOPATH', "#{HOME_DIR}/gopath:$GOPATH"
set 'PATH', "$PATH:$GOPATH/bin"

This comment has been minimized.

@joshk

joshk Oct 18, 2014
Contributor

does GVM do this for us?

This comment has been minimized.

@zackkitzmiller

zackkitzmiller Oct 18, 2014
Author Contributor

I'm honestly not sure. Ive never used gvm.

This comment has been minimized.

@BanzaiMan

BanzaiMan Oct 18, 2014
Member

$GOPATH is set to /home/travis/gopath:/home/travis/.gvm/pkgsets/go1.3.3/global, and /home/travis/.gvm/pkgsets/go1.3.3/global/bin is on $PATH.

This comment has been minimized.

@zackkitzmiller

zackkitzmiller Oct 18, 2014
Author Contributor

Cool. I can kill it then.

This comment has been minimized.

@meatballhat

meatballhat Oct 19, 2014
Contributor

It should stay, but should not treat $GOPATH as singular, meaning it should be:

       set 'PATH', '$HOME/gopath/bin:$PATH'

This comment has been minimized.

@zackkitzmiller

zackkitzmiller Oct 19, 2014
Author Contributor

👍 On it.

This comment has been minimized.

@meatballhat

meatballhat Oct 19, 2014
Contributor

😻

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

I'm actually not convinced that this works. A failure occurs in a .travis.yml that looks like this:

language: go

go: 1.3.1

install:
  - go get github.com/tools/godep
  - godep restore

script:
  - go test
  - test -z "$(go fmt ./...)"

It has to be modified to include $PATH=$PATH:$GOPATH/bin before it can find the godep command.

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 19, 2014

I think the go defaults should be modified to prepend ~/gopath/bin to
$PATH, given we're already setting ~/gopath to be the first entry in
$GOPATH. I've consistently had to do this prepending myself when
installing things like godep and using them without fully qualifying the
executable path.
On Oct 19, 2014 9:16 AM, "Zack Kitzmiller" notifications@github.com wrote:

I'm actually not convinced that this works. A failure occurs in a
.travis.yml that looks like this:

language: go
go: 1.3.1
install:

  • go get github.com/tools/godep
  • godep restore
    script:
  • go test
  • test -z "$(go fmt ./...)"

It has to be modified to include $PATH=$PATH:$GOPATH/bin before it can
find the godep command.


Reply to this email directly or view it on GitHub
#309 (comment)
.

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

@meatballhat This PR adds $GOPATH/bin to $PATH as a default. Is that not he behavior that you'd like. If not, can you let me know how you'd like it to look, and I can modify it.

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

I've adjusted the PATH as recommended.

@@ -41,6 +42,10 @@ def setup

def install
uses_make? then: 'true', else: "#{go_get} #{gobuild_args} ./...", fold: 'install', retry: true
self.if '-f Godeps/Godeps.json' do |sub|
sub.cmd "#{go_get} github.com/tools/godep", echo: true, retry: true, timing: true, assert: true
sub.cmd "godep restore", retry: true, timing: true, assert: true, echo: true

This comment has been minimized.

@meatballhat

meatballhat Oct 19, 2014
Contributor

dedent these two lines by 2 spaces

This comment has been minimized.

@meatballhat

meatballhat Oct 19, 2014
Contributor

I just took care of this. Please ignore 😸

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 19, 2014

w00t! Merging this now + deploying to staging.

meatballhat added a commit that referenced this pull request Oct 19, 2014
Provide built-in Godep support
@meatballhat meatballhat merged commit fe56367 into travis-ci:master Oct 19, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
meatballhat pushed a commit that referenced this pull request Oct 19, 2014
Dan Buch
meatballhat added a commit to meatballhat/yolo-octo-adventure that referenced this pull request Oct 19, 2014
@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 19, 2014

Appears to work 👏 https://staging.travis-ci.org/meatballhat/yolo-octo-adventure/jobs/392466

@joshk @BanzaiMan any opinions on the UX? Any folds necessary?

meatballhat added a commit to meatballhat/yolo-octo-adventure that referenced this pull request Oct 19, 2014
@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 19, 2014

Looks like we need to disable this below go1.1: https://staging.travis-ci.org/meatballhat/yolo-octo-adventure/jobs/392469

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 19, 2014

@zackkitzmiller I need to get back to GiveCamp. Any chance you can submit a follow up PR to disable when the version is under 1.1?

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

Will do. Need about an hour, I'm out at the moment.

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 19, 2014

@zackkitzmiller 👍 thank you!

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

Made a new PR. Thanks guys.

@kr
Copy link

@kr kr commented Oct 19, 2014

Hey, I'm a little late here, but I think it would be better to edit $GOPATH to put $PROJECT/Godeps/_workspace in the front, and not run godep restore. (You don't even need godep installed or available to do this.) This is why we vendor the code, so you don't have to fetch it off the internet every time.

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 19, 2014

@kr I'm a fan 👍 @zackkitzmiller willing to PR it up? 😺

@kr
Copy link

@kr kr commented Oct 19, 2014

Oh and regardless of my previous comment, thanks so much for doing this!!! So awesome. :)

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

Hey all, so I think that's a great option, and we should add it, but there are a lot of people that have a .gitignore that looks like this:

Godeps/*
!Godeps/Godeps.json

And while several project do vendor everything, not all do. Can we come up with a solution that satisfies everyone?

@kr
Copy link

@kr kr commented Oct 19, 2014

Personally I would rather not encourage that sort of gitignore, so I suggest not adding code to accommodate it. But I understand if you want to. Does anyone have real numbers for projects that do this vs ones that commit all the vendored code?

If both styles must be supported, perhaps check for the existence of Godeps/_workspace and fall back to godep restore if the directory isn't there.

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

I understand the hesitation to encourage that sort of .gitignore, but I've seen it in several projects. Enough to suggest we support the fallback solution. I can code that up a some point tomorrow.

I think that's reasonable.

@kr
Copy link

@kr kr commented Oct 19, 2014

Until today, those gitignore projects had to code up their own travis.yml support anyway, so it's not like they're losing much by having that state of affairs continue. The most important thing in good product management is knowing how to say "no". (Ok I think that's all I have to say on the subject. I also agree that the fallback is reasonable, and it's your call of course. 😄)

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 19, 2014

@kr Then for now, I'll build in a fallback. ;)

I do agree with the heart of your post, and believe we should encourage people who are vendoring to properly vendor. In the mean time, I think we should lower the friction to automated testing as to as close to zero as possible.

@kr
Copy link

@kr kr commented Oct 20, 2014

Cool 👍. Thanks so much for doing the actual work while I sit here lobbing comments from the peanut gallery. ;)

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 20, 2014

@kr ❤️

1 similar comment
@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 20, 2014

@kr ❤️

@BanzaiMan
Copy link
Member

@BanzaiMan BanzaiMan commented Oct 20, 2014

Popcorn

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 20, 2014

fwiw, I'd like to suggest (if it's at all possible without appearing to be self-promoting) that we add support for deppy, which is the -copy=false descendant of godep. This is the tool that a small number of us in the community have switched to since godep is dropping support for this way of doing things. I asked @kr a few weeks ago to see if anybody else had picked up the -copy=false fork, so if the answer has changed in the meantime I'll gladly point folks that way instead of letting deppy live on. And with that, I'll drop the subject since this thread is about godep 😸

@joshk
Copy link
Contributor

@joshk joshk commented Oct 20, 2014

I love you all, so much love in this thread!

@zackkitzmiller
Copy link
Contributor Author

@zackkitzmiller zackkitzmiller commented Oct 20, 2014

@meatballhat Respectfully, I believe that should be a new issue/PR.

@meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Oct 20, 2014

@zackkitzmiller very much agreed 😃

meatballhat pushed a commit that referenced this pull request Oct 20, 2014
BanzaiMan added a commit that referenced this pull request Oct 20, 2014
Godep integration behavior updates per feedback from @kr on #309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.