-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
make the install steps more clear in README.md #14
Conversation
@@ -5,7 +5,8 @@ | |||
### Building: | |||
|
|||
```bash | |||
git clone https://github.com/opencontainers/runc | |||
git clone https://github.com/opencontainers/runc $(GOPATH)/github.com/opencontainers/runc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$GOPATH/src to be compatible with go get.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the Godeps directory is existed in the repository, and the go get
is slow and unreliable, as it does not provide repeatable builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but other go get packages are generally under the src directory hence just to be consistent :)
LGTM |
@@ -5,7 +5,8 @@ | |||
### Building: | |||
|
|||
```bash | |||
git clone https://github.com/opencontainers/runc | |||
git clone https://github.com/opencontainers/runc $GOPATH/src/github.com/opencontainers/runc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work for all GOPATH
setups. I'd recommend go get -d
to do a download only fetch into the GOPATH
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work for all GOPATH setups.
Could you please specify why it will not work for all GOPATH
setups?
I'd recommend go get -d to do a download only fetch into the GOPATH.
As I mentioned, I noticed that the Godeps directory is existed in the repository, and the go get is slow and unreliable, as it does not provide repeatable builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Tue, Jun 23, 2015 at 03:41:52PM -0700, Lei Xue wrote:
-git clone https://github.com/opencontainers/runc
+git clone https://github.com/opencontainers/runc $GOPATH/src/github.com/opencontainers/runcThis doesn't work for all GOPATH setups.
Could you please specify why it will not work for all
GOPATH
setups?
GOPATH is a delimited list of tree roots 1. So
/path/to/a:/path/to/b is a valid GOPATH, but ‘git clone’ isn't going
to like /path/to/a:/path/to/b/src/github.com/opencontainers/runc ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it depends do we expect the audience of the README to potentially be new to go. Note http://golang.org/doc/code.html uses $GOPATH
Users who have setup a more complex GOPATH probably know what they are doing and can adjust the command. So I think it's fine to have the simple case in the README Alternately one could use
go get -d github.com/opencontainers/runc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also as remove largest suffix prefix is POSIX defined http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_03_01 we can also use
${GOPATH%%:*}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carmark go get -d github.com/opencontainers/runc
and git clone https://github.com/opencontainers/runc $GOPATH/src/github.com/opencontainers/runc
are roughly equivalent, except go get
correctly handles GOPATH
. We should avoid path munging in this build file.
Please carefully read Writing Go Code for more information on correctly setting up your development environment and managing GOPATH
. Most build problems with Go stem from not following those concepts carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you to clarify this, please close this PR if it is unnecessary to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carmark I think we can merge this if you make the suggested change to use go get -d
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevvooe
Thanks, I have updated it based on the suggested change, could you please review it and merge it?
LGTM |
make the install steps more clear in README.md
@@ -5,7 +5,8 @@ | |||
### Building: | |||
|
|||
```bash | |||
git clone https://github.com/opencontainers/runc | |||
go get -d github.com/opencontainers/runc | |||
cd $GOPATH/src/github.com/opencontainers/runc/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this fail if GOPATH is a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will fail, this isn't the right thing to do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my PR ( #23) I just put:
#cd to runc
dir
not ideal, but at least its accurate. Thoughts?
README: Remove blank line from infrastructure-agnostic paragraph
No description provided.