README.md: how to install image-tools (Issue #91) #92

Sn0rt · 2016-12-01T09:44:58Z

This is a draft for issue #91 .

Signed-off-by: Sn0rt wanggh-fnst@cn.fujitsu.com

wking · 2016-12-01T16:03:24Z

README.md

+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


We've been using $ prompts for shell-session examples (e.g. here), otherwise this looks good to me.

Longer term, we may want an installation target like this.

thx, updated.

wking · 2016-12-02T06:54:24Z

README.md

+$ skopeo copy docker://busybox oci:busybox-oci
+$ mkdir busybox-bundle
+$ oci-unpack --ref latest busybox-oci busybox-bundle
+tree busybox-bundle


Missing a prompt for this line.

wking · 2016-12-02T06:59:33Z

Makefile

+MAN := \
+	oci-create-runtime-bundle.1 \
+	oci-image-validate.1 \
+	oci-unpack.1


Stay DRY with patsubst:

MAN := $(patsubst %,%.1,$(TOOLS))

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

Or more legibly with a substitution reference:

MAN := $(TOOLS:%=%.1)

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.

wking · 2016-12-02T07:00:48Z

Makefile

+	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


No need for the phony target, just use:

install: $(TOOLS) $(MAN) …

May be some one not want to install acquiescently that meas is just compile, and we should persist a target ($tools) for developer.

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.

thx, I forget it, but the source code is always compiled even if not be updated.

wking · 2016-12-02T07:02:44Z

Makefile

+
+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


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.

The patsubst approach here is also more legible as a substitution reference:

rm -f $(MAN:%=/usr/local/share/man/man1/%) $(TOOLS:%=/usr/local/bin/%)

wking · 2016-12-02T07:24:18Z

Makefile

+_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"


You can use a static pattern rules and secondary expansion to avoid the phony:

.SECONDEXPANSION: $(MAN): %.1: cmd/$$*/$$*.1.md go-md2man -in "$<" -out "$@"

May be it can't work normally .

wking · 2016-12-02T07:40:34Z

Makefile

@@ -29,6 +34,21 @@ tools: $(TOOLS)
 $(TOOLS): oci-%:
 	go build -ldflags "-X main.gitCommit=${COMMIT}" ./cmd/$@

+.PHONY: _man install uninstall


These should go in the existing .PHONY to keep things collected.

move all of .PHONY be together ? L58 L78 L47

I think new entries should go in the .PHONY at the end. I don't have an opinion on collecting existing .PHONYs ;).

Sn0rt · 2016-12-02T09:45:02Z

The reason for append the all target is that some one install the oci tools permission be need and compile is not.

sudo make all
go build -ldflags "-X main.gitCommit=94173c11c8439e462cf9eab6d6bfab897b3da179" ./cmd/oci-create-runtime-bundle
/bin/sh: go: command not found
Makefile:38: recipe for target 'oci-create-runtime-bundle' failed

make all
go build -ldflags "-X main.gitCommit=94173c11c8439e462cf9eab6d6bfab897b3da179" ./cmd/oci-create-runtime-bundle
go build -ldflags "-X main.gitCommit=94173c11c8439e462cf9eab6d6bfab897b3da179" ./cmd/oci-image-validate
go build -ldflags "-X main.gitCommit=94173c11c8439e462cf9eab6d6bfab897b3da179" ./cmd/oci-unpack
go-md2man -in "cmd/oci-unpack/oci-unpack.1.md" -out "oci-create-runtime-bundle.1"
go-md2man -in "cmd/oci-unpack/oci-unpack.1.md" -out "oci-image-validate.1"
go-md2man -in "cmd/oci-unpack/oci-unpack.1.md" -out "oci-unpack.1"

wking · 2016-12-02T10:15:54Z

Makefile


 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"


You should document man here too.

thx , fixed

wking · 2016-12-02T10:17:54Z

Makefile

 $(TOOLS): oci-%:
 	go build -ldflags "-X main.gitCommit=${COMMIT}" ./cmd/$@

+$(MAN): %.1: cmd/*/*.1.md


This does not work (see here, where all of your man pages are built from oci-unpack.1.md). You want to use secondary expansion (with the line I suggested here).

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.

$(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

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.

sogar, thx! fixed.

wking · 2016-12-02T10:18:57Z

Makefile

+	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))


This would read more easily as a substitution reference (with the line I suggested here).

thx very much, fixed

wking · 2016-12-02T10:19:52Z

Makefile

-	test
+	test \
+	$(MAN) \
+	$(TOOLS)


Neither $(MAN) nor $(TOOLS) are phony. They are all real files that we're building, and they all have targets pointing at them.

wking · 2016-12-02T15:31:26Z

Makefile

+$(MAN): %.1: cmd/$$*/$$*.1.md
+	go-md2man -in "$<" -out "$@"
+
+install:


This depends on $(TOOLS) and $(MAN).

I think that the installation is a special action which depend on permissions, and building is not need permissions.

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.

wking · 2016-12-04T00:02:57Z

README.md

+Recommend to use `go` to download.
+
+```
+$ go get github.com/opencontainers/image-tools/


$ 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

I consider that It does not matter.

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).

wking · 2016-12-04T00:25:17Z

README.md

+
+## example
+
+Firstly, May be you need to install `skopeo`  and more information at [here](https://github.com/projectatomic/skopeo).


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.

…

thx, fixed.

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.

You're right

Sn0rt · 2016-12-05T08:27:02Z

PTAL

wking · 2016-12-06T08:51:36Z

README.md

+
+### 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.


Backticks around busybox-oci?

wking · 2016-12-21T05:24:27Z

README.md

+it is recommended that use `go` to download a single command tools.
+
+```
+$ go get -d github.com/opencontainers/image-tools/cmd/oci-unpack 


Trailing whitespace.

wking · 2016-12-21T15:28:44Z

bffd46b looks good to me :).

Sn0rt · 2016-12-22T02:43:22Z

@xiekeyang @coolljt0725 PTAL

xiekeyang · 2016-12-22T07:08:15Z

README.md

+
+### 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`.


The syntax of URL link seems incorrect. Should be [abbreviation]--(url)
And please check other positions in this commit.

Extremely embarrassing, fixed.

coolljt0725 · 2016-12-22T08:06:13Z

Makefile


 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"


To keep consistent, remove -> Remove

fixed, thx very much.

coolljt0725 · 2016-12-22T08:11:52Z

README.md

@@ -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


I think this should be ## Install , double #

coolljt0725 · 2016-12-22T08:12:56Z

README.md

@@ -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.


The first letter should be capital
go -> go get?

coolljt0725 · 2016-12-22T08:14:11Z

README.md

+$ sudo make install
+```
+
+## uninstall


To keep consistent, capitalize first letter. Uninstall

coolljt0725 · 2016-12-22T08:14:19Z

README.md

+$ sudo make uninstall
+```
+
+## example


coolljt0725 · 2016-12-22T08:31:33Z

Makefile

 $(TOOLS): oci-%:
 	go build -ldflags "-X main.gitCommit=${COMMIT}" ./cmd/$@

+.SECONDEXPANSION:
+$(MAN): %.1: cmd/$$*/$$*.1.md
+	go-md2man -in "$<" -out "$@"


should we install go-md2man in install.tools https://github.com/opencontainers/image-tools/blob/master/Makefile#L60?

I consider we should install it by install.tools , it seems consistent with the front.

Signed-off-by: Sn0rt <wanggh-fnst@cn.fujitsu.com>

Sn0rt · 2016-12-29T06:42:59Z

PTAL again

coolljt0725 · 2017-01-22T04:15:45Z

LGTM

xiekeyang · 2017-02-07T02:37:45Z

LGTM

cc @opencontainers/image-tools-maintainers

vbatts · 2017-02-08T19:18:04Z

LGTM

wking reviewed Dec 1, 2016

View reviewed changes

wking mentioned this pull request Dec 1, 2016

ReadMe should include description of how to install #91

Closed

Sn0rt force-pushed the update-readme-with-install branch 3 times, most recently from b19267f to b727a22 Compare December 2, 2016 04:53

wking reviewed Dec 2, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch 2 times, most recently from 5d4b69e to e768c01 Compare December 2, 2016 06:57

wking reviewed Dec 2, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch 3 times, most recently from 3a3deba to 94173c1 Compare December 2, 2016 09:35

wking reviewed Dec 2, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch 3 times, most recently from 4caa656 to f14a9b6 Compare December 2, 2016 15:21

wking reviewed Dec 2, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch from f14a9b6 to 3406e6a Compare December 3, 2016 12:10

wking reviewed Dec 4, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch 2 times, most recently from a7cba1a to 9dde9ac Compare December 5, 2016 03:06

wking reviewed Dec 6, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch from 9dde9ac to f3b43ab Compare December 6, 2016 09:38

Sn0rt force-pushed the update-readme-with-install branch from f3b43ab to b5aca91 Compare December 21, 2016 04:54

wking reviewed Dec 21, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch from b5aca91 to bffd46b Compare December 21, 2016 05:32

xiekeyang reviewed Dec 22, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch from bffd46b to 6f1caa7 Compare December 22, 2016 07:26

coolljt0725 reviewed Dec 22, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch from 6f1caa7 to 183049c Compare December 22, 2016 08:30

coolljt0725 reviewed Dec 22, 2016

View reviewed changes

Sn0rt force-pushed the update-readme-with-install branch 3 times, most recently from 223cb52 to 32eaa3b Compare December 22, 2016 09:37

README.md: how to install image-tools

32eaa3b

Signed-off-by: Sn0rt <wanggh-fnst@cn.fujitsu.com>

vbatts merged commit 945a4e1 into opencontainers:master Feb 8, 2017

coolljt0725 mentioned this pull request Feb 9, 2017

[README.md] Add detail description of oci-image-tool #48

Closed

xiekeyang mentioned this pull request Feb 16, 2017

Bump version for v0.1.0 #116

Merged


		## example

		Firstly, May be you need to install `skopeo` and more information at [here](https://github.com/projectatomic/skopeo).


		### 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.

		@@ -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

README.md: how to install image-tools (Issue #91) #92

README.md: how to install image-tools (Issue #91) #92

Conversation

Sn0rt commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt commented Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Obtaining an image

oci-create-runtime-bundle

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Dec 21, 2016

Sn0rt commented Dec 22, 2016

xiekeyang Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sn0rt Dec 2, 2016 •

edited

Loading

Sn0rt Dec 2, 2016 •

edited

Loading

Sn0rt Dec 2, 2016 •

edited

Loading

Sn0rt Dec 2, 2016 •

edited

Loading

Sn0rt commented Dec 2, 2016 •

edited

Loading

Sn0rt Dec 2, 2016 •

edited

Loading

Sn0rt Dec 2, 2016 •

edited

Loading

Sn0rt Dec 2, 2016 •

edited

Loading

xiekeyang Dec 22, 2016 •

edited

Loading

coolljt0725 commented Jan 22, 2017 •

edited by caniszczyk

Loading

xiekeyang commented Feb 7, 2017 •

edited by caniszczyk

Loading

vbatts commented Feb 8, 2017 •

edited by caniszczyk

Loading