-
Notifications
You must be signed in to change notification settings - Fork 82
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
README.md: how to install image-tools (Issue #91) #92
Conversation
go get github.com/opencontainers/image-tools/ | ||
cd $GOPATH/src/github.com/opencontainers/image-tools/ | ||
make tools | ||
cp oci-unpack oci-create-runtime-bundle oci-image-validate /usr/local/bin |
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.
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.
thx, updated.
b19267f
to
b727a22
Compare
$ skopeo copy docker://busybox oci:busybox-oci | ||
$ mkdir busybox-bundle | ||
$ oci-unpack --ref latest busybox-oci busybox-bundle | ||
tree busybox-bundle |
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.
Missing a prompt for this line.
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.
fixed
5d4b69e
to
e768c01
Compare
MAN := \ | ||
oci-create-runtime-bundle.1 \ | ||
oci-image-validate.1 \ | ||
oci-unpack.1 |
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.
Stay DRY with patsubst
:
MAN := $(patsubst %,%.1,$(TOOLS))
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.
The MAN is consistent with TOOLS named.
TOOLS := \
oci-create-runtime-bundle \
oci-image-validate \
oci-unpack
MAN := \
oci-create-runtime-bundle.1 \
oci-image-validate.1 \
oci-unpack.1
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.
Or more legibly with a substitution reference:
MAN := $(TOOLS:%=%.1)
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.
The MAN is consistent with TOOLS named.
Right, but using the substitution reference lets you define the pattern “all tools have a %.1 man page” instead of re-listing all the tool names. That means there's only one place to touch (TOOLS
) when you add/remove new commands.
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.
fixed
go-md2man -in "cmd/oci-image-validate/oci-image-validate.1.md" -out "oci-image-validate.1" | ||
go-md2man -in "cmd/oci-create-runtime-bundle/oci-create-runtime-bundle.1.md" -out "oci-create-runtime-bundle.1" | ||
|
||
install: _man |
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.
No need for the phony target, just use:
install: $(TOOLS) $(MAN)
…
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.
May be some one not want to install acquiescently that meas is just compile, and we should persist a target ($tools
) for developer.
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.
Phony targets as leaf-targets are fine. But if you put them in a chain you'll always rebuild leafier targets. So I'd like to see:
.PHONY: … install man tools …
man: $(MAN)
tools: $(TOOLS)
install: $(MAN) $(TOOLS)
…
That way you don't re-install if none of the sources changed.
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.
thx, I forget it, but the source code
is always compiled even if not be updated.
|
||
uninstall: clean | ||
rm -f /usr/local/share/man/man1/oci-create-runtime-bundle.1 /usr/local/share/man/man1/oci-image-validate.1 /usr/local/share/man/man1/oci-unpack.1 | ||
rm -f /usr/local/bin/oci-create-runtime-bundle /usr/local/bin/oci-image-validate /usr/local/bin/oci-unpack |
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.
rm -f $(patsubst %,/usr/local/share/man/man1/%,$(MAN)) $(patsubst %,/usr/local/bin/%,$(TOOLS))
although you probably want variables for /usr/local/share/man
and /usr/local/bin/
in case the user wants to override them.
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.
The patsubst approach here is also more legible as a substitution reference:
rm -f $(MAN:%=/usr/local/share/man/man1/%) $(TOOLS:%=/usr/local/bin/%)
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.
thx, fixed
_man: | ||
go-md2man -in "cmd/oci-unpack/oci-unpack.1.md" -out "oci-unpack.1" | ||
go-md2man -in "cmd/oci-image-validate/oci-image-validate.1.md" -out "oci-image-validate.1" | ||
go-md2man -in "cmd/oci-create-runtime-bundle/oci-create-runtime-bundle.1.md" -out "oci-create-runtime-bundle.1" |
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.
You can use a static pattern rules and secondary expansion to avoid the phony:
.SECONDEXPANSION:
$(MAN): %.1: cmd/$$*/$$*.1.md
go-md2man -in "$<" -out "$@"
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.
May be it can't work normally .
@@ -29,6 +34,21 @@ tools: $(TOOLS) | |||
$(TOOLS): oci-%: | |||
go build -ldflags "-X main.gitCommit=${COMMIT}" ./cmd/$@ | |||
|
|||
.PHONY: _man install uninstall |
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.
These should go in the existing .PHONY
to keep things collected.
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.
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 think new entries should go in the .PHONY at the end. I don't have an opinion on collecting existing .PHONYs ;).
3a3deba
to
94173c1
Compare
The reason for append the
|
|
||
default: help | ||
|
||
help: | ||
@echo "Usage: make <target>" | ||
@echo | ||
@echo " * 'all' - Build the oci tools and manual pages" | ||
@echo " * 'install' - Install binaries and manual pages" | ||
@echo " * 'uninstall' - remove the oci tools and manual pages" | ||
@echo " * 'tools' - Build the oci image tools binaries" |
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.
You should document man
here too.
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.
thx , fixed
$(TOOLS): oci-%: | ||
go build -ldflags "-X main.gitCommit=${COMMIT}" ./cmd/$@ | ||
|
||
$(MAN): %.1: cmd/*/*.1.md |
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.
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.
diff --git a/Makefile b/Makefile
index c564720..bd6ad0b 100644
--- a/Makefile
+++ b/Makefile
@@ -38,7 +38,7 @@ all: $(TOOLS) $(MAN)
$(TOOLS): oci-%:
go build -ldflags "-X main.gitCommit=${COMMIT}" ./cmd/$@
-$(MAN): %.1: cmd/*/*.1.md
+$(MAN): %.1: cmd/$$*/$$*.1.md
go-md2man -in "$<" -out "$@"
make man
go-md2man -in "" -out "oci-create-runtime-bundle.1"
it will be hang.
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.
$(MAN): %.1: cmd/*/*.1.md
go-md2man -in "$<" -out "$@"
make man
go-md2man -in "cmd/oci-create-runtime-bundle/oci-create-runtime-bundle.1.md" -out "oci-create-runtime-bundle.1"
go-md2man -in "cmd/oci-create-runtime-bundle/oci-create-runtime-bundle.1.md" -out "oci-image-validate.1"
go-md2man -in "cmd/oci-create-runtime-bundle/oci-create-runtime-bundle.1.md" -out "oci-unpack.1"
it's ok
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.
it's ok
It's not ok, it's using oci-create-runtime-bundle.1.md
as the source for all the pages.
it will be hang
That's because you left off the SECONDEXPANSION:
line. It's important ;). Doc link in my original secondary-expansion comment.
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.
sogar, thx! fixed.
install -m 644 $(MAN) /usr/local/share/man/man1 | ||
|
||
uninstall: clean | ||
rm -f $(patsubst %,/usr/local/share/man/man1/%,$(MAN)) $(patsubst %,/usr/local/bin/%,$(TOOLS)) |
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 would read more easily as a substitution reference (with the line I suggested here).
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.
thx very much, fixed
test | ||
test \ | ||
$(MAN) \ | ||
$(TOOLS) |
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.
Neither $(MAN)
nor $(TOOLS)
are phony. They are all real files that we're building, and they all have targets pointing at them.
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.
updated
4caa656
to
f14a9b6
Compare
$(MAN): %.1: cmd/$$*/$$*.1.md | ||
go-md2man -in "$<" -out "$@" | ||
|
||
install: |
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 depends on $(TOOLS)
and $(MAN)
.
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 think that the installation is a special action which depend on permissions, and building is not need permissions.
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 think that the installation is a special action which depend on permissions, and building is not need permissions.
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.
Users making separate calls to build and install is fine, but installation still depends on having the prerequisites available. By listing them, you avoid confusing users who do decide to jump straight in with install
.
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.
ok, fixed.
f14a9b6
to
3406e6a
Compare
Recommend to use `go` to download. | ||
|
||
``` | ||
$ go get github.com/opencontainers/image-tools/ |
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.
$ go get github.com/opencontainers/image-tools/
package github.com/opencontainers/image-tools: no buildable Go source files in /tmp/whatever/src/github.com/opencontainers/image-tools
$ echo $?
1
It's probably better to pick a single command:
$ go get -d github.com/opencontainers/image-tools/cmd/oci-unpack
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 consider that It does not matter.
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.
It matters, because the -d …path/to/cmd…
approach succeeds (exits zero), but the approach you have now fails (see the warning and the 1 returned by echo $?
in my comment above).
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.
fixed
|
||
## example | ||
|
||
Firstly, May be you need to install `skopeo` and more information at [here](https://github.com/projectatomic/skopeo). |
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.
All of these examples are using a busybox-oci
image. I'd probably structure it like:
Obtaining an image
The following examples assume you have a image-layout tar archive at busybox-oci
.
One way to acquire that image is with skopeo:
$ skopeo copy docker://busybox oci:busybox-oci
oci-create-runtime-bundle
$ mkdir busybox-bundle
$ oci-create-runtime-bundle --ref latest busybox-oci busybox-bundle
$ cd busybox-bundle && sudo runc run busybox
More information about oci-create-runtime-bundle
can be found in its man page.
…
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.
thx, fixed.
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.
With the new “Obtaining an image” linking to skopeo's installation section, I don't think we need any of the text between the “example” and “Obtaining an image” headers anymore. Folks who don't have skopeo
can go over to that project's installation docs and see how to install it without us duplicating their instructions here.
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.
You're right
a7cba1a
to
9dde9ac
Compare
PTAL |
|
||
### Obtaining an image | ||
|
||
The following examples assume you have a (image-layout)[https://github.com/opencontainers/image-spec/blob/v1.0.0-rc2/image-layout.md] tar archive at busybox-oci. |
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.
Backticks around busybox-oci
?
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.
fixed
9dde9ac
to
f3b43ab
Compare
f3b43ab
to
b5aca91
Compare
it is recommended that use `go` to download a single command tools. | ||
|
||
``` | ||
$ go get -d github.com/opencontainers/image-tools/cmd/oci-unpack |
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.
b5aca91
to
bffd46b
Compare
bffd46b looks good to me :). |
@xiekeyang @coolljt0725 PTAL |
|
||
### Obtaining an image | ||
|
||
The following examples assume you have a (image-layout)[https://github.com/opencontainers/image-spec/blob/v1.0.0-rc2/image-layout.md] tar archive at `busybox-oci`. |
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.
The syntax of URL link seems incorrect. Should be [abbreviation]--(url)
And please check other positions in this commit.
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.
Extremely embarrassing, fixed.
bffd46b
to
6f1caa7
Compare
|
||
default: help | ||
|
||
help: | ||
@echo "Usage: make <target>" | ||
@echo | ||
@echo " * 'all' - Build the oci tools and manual pages" | ||
@echo " * 'install' - Install binaries and manual pages" | ||
@echo " * 'uninstall' - remove the oci tools and manual pages" |
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.
To keep consistent, remove
-> Remove
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.
fixed, thx very much.
@@ -2,6 +2,71 @@ | |||
|
|||
`image-tools` is a collection of tools for working with the [OCI image format specification](https://github.com/opencontainers/image-spec). | |||
|
|||
# Install |
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 think this should be ## Install
, double #
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.
got
@@ -2,6 +2,71 @@ | |||
|
|||
`image-tools` is a collection of tools for working with the [OCI image format specification](https://github.com/opencontainers/image-spec). | |||
|
|||
# Install | |||
|
|||
it is recommended that use `go` to download a single command tools. |
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.
The first letter should be capital
go
-> 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.
copy that
$ sudo make install | ||
``` | ||
|
||
## uninstall |
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.
To keep consistent, capitalize first letter. Uninstall
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.
copy that
$ sudo make uninstall | ||
``` | ||
|
||
## example |
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.
Example
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.
fixed
6f1caa7
to
183049c
Compare
$(TOOLS): oci-%: | ||
go build -ldflags "-X main.gitCommit=${COMMIT}" ./cmd/$@ | ||
|
||
.SECONDEXPANSION: | ||
$(MAN): %.1: cmd/$$*/$$*.1.md | ||
go-md2man -in "$<" -out "$@" |
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.
should we install go-md2man
in install.tools
https://github.com/opencontainers/image-tools/blob/master/Makefile#L60?
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 consider we should install it by install.tools , it seems consistent with the front.
223cb52
to
32eaa3b
Compare
Signed-off-by: Sn0rt <wanggh-fnst@cn.fujitsu.com>
PTAL again |
This is a draft for issue #91 .
Signed-off-by: Sn0rt wanggh-fnst@cn.fujitsu.com