Add make install support #31
Conversation
Makefile
Outdated
| install: | ||
| install -D -m 755 oci-create-runtime-bundle ${INSTALL_DIR} | ||
| install -D -m 755 oci-unpack ${INSTALL_DIR} | ||
| install -D -m 755 oci-image-validate ${INSTALL_DIR} |
There was a problem hiding this comment.
I'm not excited about repeatedly listing all of our compiled tools. I don't see a way around that here, but with #28 we could do:
install: $(TOOLS)
install -D -t $(INSTALL_DIR) $^
There was a problem hiding this comment.
Seems better. I can wait until #28 is merged. 😄
There was a problem hiding this comment.
well, more $(DESTDIR) which is more the autoconf standard
WeiZhang555
force-pushed
the
make-install
branch
from
2bd5849 to
ee2ca51
Compare
.travis.yml
Outdated
| - make check-license | ||
| - make test | ||
| - make tools | ||
| - sudo make install |
There was a problem hiding this comment.
do we need to have sudo here, if we just add a DESTDIR variable so it can be installed to a limited user tmp dir. Maybe. Just thinking out loud here.
There was a problem hiding this comment.
.+1 to using DESTDIR and dropping sudo.
WeiZhang555
force-pushed
the
make-install
branch
4 times, most recently
from
f9fab1f to
131575b
Compare
.travis.yml
Outdated
| - go get -u github.com/alecthomas/gometalinter | ||
| - gometalinter --install --update | ||
| - go get -t -d ./... | ||
| - mkdir /tmp/image-tools -p |
There was a problem hiding this comment.
Can we use mkdir -p /tmp/image-tools? The POSIX template is:
mkdir [-p] [-m mode] dir…
131575b to
b7c33d8
Compare
|
b7c33d8 looks good to me.
|
|
Any suggestion? @vbatts |
.travis.yml
Outdated
|
|
||
| before_script: | ||
| - export PATH=$HOME/gopath/bin:$PATH | ||
| - export DESTDIR=/tmp/image-tools |
There was a problem hiding this comment.
This works, but I guess I would likely just include this with the command
.travis.yml
Outdated
| - make check-license | ||
| - make test | ||
| - make tools | ||
| - make install |
There was a problem hiding this comment.
right here, like make install DESTDIR=/tmp/image-tools
WeiZhang555
force-pushed
the
make-install
branch
from
b7c33d8 to
a825aa8
Compare
|
@vbatts Updated. |
.travis.yml
Outdated
| - make check-license | ||
| - make test | ||
| - make tools | ||
| - DESTDIR=/tmp/image-tools make install |
There was a problem hiding this comment.
Usually the variable is set after the targets as in @vbatts' example or the POSIX make usage's macro=value… or in the GNU make docs for overriding variables. POSIX make (not that we support it) requires an -e before environment variables will override macros. GNU make has more complicated environment → variable logic. But it seems best here to avoid the environment-variable approach entirely and to use the make install DESTDIR=/tmp/image-tools phrasing instead.
There was a problem hiding this comment.
All right, it's quite convincible.
I'll update, thanks 😄
WeiZhang555
force-pushed
the
make-install
branch
from
a825aa8 to
b5e5da5
Compare
.travis.yml
Outdated
| - make check-license | ||
| - make test | ||
| - make tools | ||
| - make install DESTDIR=/tmp/image-tools |
There was a problem hiding this comment.
My bad. It's green now 😄
Add `install` support to make, for installing image tools binaries into $PATH. Signed-off-by: Zhang Wei <[email protected]>
WeiZhang555
force-pushed
the
make-install
branch
from
b5e5da5 to
2fa43fe
Compare
|
2fa43fe looks good to me :). |
|
ping @vbatts |
|
LGTM |
|
LGTM |
|
Since this has been dangling for so long, I'm closing this. |
|
On Tue, Jan 10, 2017 at 01:17:45AM -0800, zhangwei_cs wrote:
Since this has been dangling for so long, I'm closing this.
The diff still looks good to me. I imagine that maintainers are just
busy with other things, and will eventually get back around to
reviewing image-tools PRs.
|
|
So maybe I should re-open this and keep it for some more time 😂 |
|
LGTM |
|
Need Rebase |
4 participants
Add
installsupport to make, for installing image tools binaries into$PATH.
Signed-off-by: Zhang Wei [email protected]