version: Add a version command #940

wking · 2016-07-11T16:56:38Z

And adjust runc --version to print exactly the same content.

The

$ COMMAND --version

interface is a popular one for this information and makes a lot of sense for single-action commands. For example, [printf(1)] is only ever going to do one meaningful thing, so --version, --help, and other command-like actions work well as options.

Umbrella commands with multiple sub-commands, on the other hand, have a more consistent interface if “... and exit” actions get their own sub-commands. That allows you to declare an interface like:

$ COMMAND [global-options] SUB-COMMAND [sub-command-specific-options] [sub-command-specific-arguments]

without having to say “except if you use a sub-command-like global option, in which case SUB-COMMAND is not allowed”.

With this commit, we add support for version while keeping --version, because while version makes more sense for umbrella commands, a user may not know if runc is an umbrella command when they ask for the version.

Existing umbrella commands are not particularly consistent:

git(1) supports both --version and version, --help and help.
btrfs(8) supports both --version and version, --help and help.
ip(1) supports -V / -Version and help.
npm supports version and help.
pip supports --version, --help and help.

See related discussion in opencontainers/runtime-spec#513.

mlaventure · 2016-07-11T18:18:03Z

Makefile

@@ -25,10 +25,10 @@ MAN_INSTALL_PATH := ${PREFIX}/share/man/man8/
 VERSION := ${shell cat ./VERSION}

 all: $(RUNC_LINK)
-	go build -i -ldflags "-X main.gitCommit=${COMMIT} -X main.version=${VERSION}" -tags "$(BUILDTAGS)" -o runc .
+	go build -i -ldflags "-X version.gitCommit=${COMMIT} -X version.version=${VERSION}" -tags "$(BUILDTAGS)" -o runc .


Revert this and the bottom change.

the first part refers to the package, not the filename.

mikebrow · 2016-07-11T18:34:47Z

Suggest adding the following diff to update the integration test bucket:

--- a/tests/integration/version.bats
+++ b/tests/integration/version.bats
@@ -2,10 +2,18 @@

 load helpers

-@test "runc version" {
+@test "runc --version" {
   runc -v
   [ "$status" -eq 0 ]
   [[ ${lines[0]} =~ runc\ version\ [0-9]+\.[0-9]+\.[0-9]+ ]]
   [[ ${lines[1]} =~ commit:+ ]]
   [[ ${lines[2]} =~ spec:\ [0-9]+\.[0-9]+\.[0-9]+ ]]
 }
+
+@test "runc version" {
+  runc version
+  [ "$status" -eq 0 ]
+  [[ ${lines[0]} =~ runc\ version\ [0-9]+\.[0-9]+\.[0-9]+ ]]
+  [[ ${lines[1]} =~ commit:+ ]]
+  [[ ${lines[2]} =~ spec:\ [0-9]+\.[0-9]+\.[0-9]+ ]]
+}

wking · 2016-07-11T19:02:13Z

Pushed fbc7490 → b8ad6a4 which:

Updates the test suite 1.
Reverts the broken ‘-X version.…’ in the Makefile 2.

hqhq · 2016-07-15T08:47:58Z

Looks good.

Jenkins failure seems unrelated, and I don't have authorization to retrigger yet. @wking Can you re-push to trigger Jenkins again?

wking · 2016-07-15T17:30:03Z

On Fri, Jul 15, 2016 at 01:48:00AM -0700, Qiang Huang wrote:

Can you re-push to trigger Jenkins again?

Rebased onto master with b8ad6a4 → 8ab786f. Jenkins is still failing,
but the error message 1:

bats -t tests/integration
/tmp/bats.1894.src: line 8: syntax error in conditional expression
/usr/local/libexec/bats-exec-suite: line 20: let: count+=: syntax error: operand expected (error token is "+=")
Makefile:68: recipe for target 'localintegration' failed

looks like sstephenson/bats#140. I suspect an old Bash or otherwise
misconfigured shell on the test box.

crosbymichael · 2016-08-23T17:46:23Z

version.go

+}
+
+func printVersion(context *cli.Context) (err error) {
+	if version == "" {


Why don't you just do the same logic with the slice and strings.Join so we don't have all these verbose if else

On Tue, Aug 23, 2016 at 10:46:25AM -0700, Michael Crosby wrote:

+// VERSION file of the source code.
+var version = ""
+
+// gitCommit will be the hash that the binary was built from
+// and will be populated by the Makefile
+var gitCommit = ""
+
+var versionCommand = cli.Command{

Name: "version",

Usage: "output the runtime version",

Description: The version command outputs the runtime version.,

Action: printVersion,
+}

+func printVersion(context *cli.Context) (err error) {

if version == "" {

Why don't you just do the same logic with the slice and strings.Join
so we don't have all these verbose if else

The same logic as what? This check is for “someone build this
withought ‘-X main.version=…’ like we have in the Makefile”, and I
think we want to keep a check for that case.

Like we had in the main.go before this change.

On Tue, Aug 23, 2016 at 11:09:30AM -0700, Michael Crosby wrote:

+// VERSION file of the source code.
+var version = ""
+
+// gitCommit will be the hash that the binary was built from
+// and will be populated by the Makefile
+var gitCommit = ""
+
+var versionCommand = cli.Command{

Name: "version",

Usage: "output the runtime version",

Description: The version command outputs the runtime version.,

Action: printVersion,
+}

+func printVersion(context *cli.Context) (err error) {

if version == "" {

Like we had in the main.go before this change.

The code I removed from main.go was:

if version != "" {

v = append(v, version)

}

if gitCommit != "" {

v = append(v, fmt.Sprintf("commit: %s", gitCommit))

}

v = append(v, fmt.Sprintf("spec: %s", specs.Version))

app.Version = strings.Join(v, "\n")

That's building the output string in memory and using Join to store it
(which you need when you're using app.Version). But with
cli.VersionPrinter, I don't think we need that single-string form
anymore, and using os.Stdout.WriteString directly avoids building the
the string in memory. Do you want me to build the string in memory to
save some WriteString err checks?

I believe that's what he's asking for, build a string in memory then output it.

crosbymichael · 2016-08-23T17:46:46Z

The build is still broken and needs to be fixed, its related to this change

wking · 2016-08-23T18:34:03Z

On Tue, Aug 23, 2016 at 10:46:47AM -0700, Michael Crosby wrote:

The build is still broken and needs to be fixed, its related to this
change

Ah, looks like I was missing an escape for a space in the expected
lines[0] comparison. Fixed with d99746a.

crosbymichael · 2016-08-23T18:51:53Z

The version is broken when just running runc and seeing the binary's main output


VERSION:
   0.0.0

wking · 2016-08-23T19:25:06Z

On Tue, Aug 23, 2016 at 11:51:55AM -0700, Michael Crosby wrote:

The version is broken when just running runc and seeing the
binary's main output
VERSION:
   0.0.0

Hmm. I'm not sure why they use .Version directly there instead of
using VersionPrinter. I see a few possible approaches:

a. Go back to the in-memory string and assign it to app.Version.

i. Don't bother with whitespace and leave the current tip's
inconsistent intend:

    VERSION:
       1.0.0-rc1
    commit: 0c6733d669165b53c0117ab522866025c70b3039
    spec: 1.0.0-rc1

ii. Prefix every line but the first with matching whitespace:

    VERSION:
       1.0.0-rc1
       commit: 0c6733d669165b53c0117ab522866025c70b3039
       spec: 1.0.0-rc1

iii. Prefix the whole Version string with a newline (although this
will leave some dangling whitespace in the “blank” line):

    VERSION:

    1.0.0-rc1
    commit: 0c6733d669165b53c0117ab522866025c70b3039
    spec: 1.0.0-rc1

b. Adjust the help template 1.

i. In combination with a.i, use:

    VERSION:

    {{.Version}}

  which would give a result like a.iii, but without the dangling
  whitespace.

ii. Drop the VERSION section entirely (folks can run ‘runc version’
or ‘runc --version’ if they want it).

iii. Adjust the template so we can call cli.VersionPrinter instead
of using {{.Version}} 2. I'm not familiar enough with Go's
template language and the cli package to know if this is
possible.

Preferences?

mikebrow · 2016-08-23T21:44:02Z

My preference is to drop the VERSION section from the default help.

And adjust 'runc --version' to print exactly the same content. The: $ COMMAND --version interface is a popular one for this information and makes a lot of sense for single-action commands. For example, printf(1) [1] is only ever going to do one meaningful thing, so --version, --help, and other command-like actions work well as options. Umbrella commands with multiple sub-commands, on the other hand, have a more consistent interface if '... and exit' actions get their own sub-commands. That allows you to declare an interface like: $ COMMAND [global-options] SUB-COMMAND [sub-command-specific-options] [sub-command-specific-arguments] without having to say "except if you use a sub-command-like global option, in which case SUB-COMMAND is not allowed". With this commit, we add support for 'version' while keeping --version, because while 'version' makes more sense for umbrella commands, a user may not know if runc is an umbrella command when they ask for the version. Existing umbrella commands are not particularly consistent: * git(1) supports both --version and 'version', --help and 'help'. * btrfs(8) supports both --version and 'version', --help and 'help'. * ip(1) supports -V / -Version and 'help'. * npm supports 'version' and 'help'. * pip supports '--version', --help and 'help'. Setting .Version to the empty string takes advantage of the {{if .Version}} conditional [2] to avoid --help showing: VERSION: 0.0.0 because we are no longer setting .Version. I floated a few options [4], and Mike Brown (the only one with a preference) was in favor of dropping the VERSION section [5] (which turned out to not need a AppHelpTemplate adjustment ;). The help tests ensure that attempting to remove "VERSION" from the help output does not change the output (i.e. that the output didn't contain "VERSION" to begin with). That protects us from subsequent urfave/cli changes breaking the empty-string .Version approach. [1]: http://www.man7.org/linux/man-pages/man1/printf.1.html [2]: https://github.com/urfave/cli/blob/v1.18.1/README.md#customization-1 [3]: opencontainers#940 (comment) [4]: opencontainers#940 (comment) [5]: opencontainers#940 (comment) Signed-off-by: W. Trevor King <[email protected]>

wking · 2016-09-08T05:34:39Z

On Tue, Aug 23, 2016 at 02:44:03PM -0700, Mike Brown wrote:

My preference is to drop the VERSION section from the default help.

Done with tests in d99746a → e09dd4f, which also rebases onto master.
It turned out that I didn't have to touch the template, and just had
to set .Version to an empty string.

crosbymichael

Overall I don't see what all we gain by adding this as there is no standard saying you have to have a version command. Most people know to add -v or --version and that works fine so I don't know why we need this and would rather not take it considering the changes added but if others feel different then I added changes that need to be made.

crosbymichael · 2016-10-31T18:49:57Z

version.go

+
+func printVersion(context *cli.Context) (err error) {
+	if version == "" {
+		_, err = os.Stdout.WriteString("runC unknown\n")


its runc not runC

It's runC on http://runc.io/. And runC sort of helps avoid #24. But whatever, I can change it ;).

crosbymichael · 2016-10-31T18:50:29Z

version.go

+	if version == "" {
+		_, err = os.Stdout.WriteString("runC unknown\n")
+	} else {
+		_, err = os.Stdout.WriteString(fmt.Sprintf("runC %s\n", version))


everything is printing with a : between the name and the version

everything is printing with a : between the name and the version

So you want one here too? I'm just matching the current master approach:

$ ./runc --version runc version 1.0.0-rc3 commit: 6b574d57594aedca4bb45b679788af73ae0d65f9 spec: 1.0.0-rc5

but I can go either way (with or without colons) if you prefer.

crosbymichael · 2016-10-31T18:51:41Z

version.go

+	Action:      printVersion,
+}
+
+func printVersion(context *cli.Context) (err error) {


this entire function's contents are poorly written and is bad quality. No where else in our code do we use these functions on os.Stdout and all the branches with if/else are just bad. It needs to be improved

No where else in our code do we use these functions on os.Stdout…

I'm replacing the os.Stdout.WriteString calls with fmt.Print* calls.

… and all the branches with if/else are just bad.

Most of the if blocks are for error checking, and I don't know how to avoid that in Go. The only else is to cover “have/don't-have a version?”, and I think we want to cover both sides of that. Besides error checking and version presence, the only other conditional is for “have a Git commit?”, and I think we want to cover that too. Do you have any conditionals you'd like to see dropped?

And adjust 'runc --version' to print exactly the same content. The: $ COMMAND --version interface is a popular one for this information and makes a lot of sense for single-action commands. For example, printf(1) [1] is only ever going to do one meaningful thing, so --version, --help, and other command-like actions work well as options. Umbrella commands with multiple sub-commands, on the other hand, have a more consistent interface if '... and exit' actions get their own sub-commands. That allows you to declare an interface like: $ COMMAND [global-options] SUB-COMMAND [sub-command-specific-options] [sub-command-specific-arguments] without having to say "except if you use a sub-command-like global option, in which case SUB-COMMAND is not allowed". With this commit, we add support for 'version' while keeping --version, because while 'version' makes more sense for umbrella commands, a user may not know if runc is an umbrella command when they ask for the version. Existing umbrella commands are not particularly consistent: * git(1) supports both --version and 'version', --help and 'help'. * btrfs(8) supports both --version and 'version', --help and 'help'. * ip(1) supports -V / -Version and 'help'. * npm supports 'version' and 'help'. * pip supports '--version', --help and 'help'. Setting .Version to the empty string takes advantage of the {{if .Version}} conditional [2] to avoid --help showing: VERSION: 0.0.0 because we are no longer setting .Version. I floated a few options [4], and Mike Brown (the only one with a preference) was in favor of dropping the VERSION section [5] (which turned out to not need a AppHelpTemplate adjustment ;). The help tests ensure that attempting to remove "VERSION" from the help output does not change the output (i.e. that the output didn't contain "VERSION" to begin with). That protects us from subsequent urfave/cli changes breaking the empty-string .Version approach. I prefer "runC" to "runc", to match http://runc.io/ and some naming issues [6], but Michael prefers "runc" [7]. [1]: http://www.man7.org/linux/man-pages/man1/printf.1.html [2]: https://github.com/urfave/cli/blob/v1.18.1/README.md#customization-1 [3]: opencontainers#940 (comment) [4]: opencontainers#940 (comment) [5]: opencontainers#940 (comment) [6]: opencontainers#24 [7]: opencontainers#940 (comment) Signed-off-by: W. Trevor King <[email protected]>

wking · 2017-03-21T23:18:55Z

@crosbymichael wrote

Overall I don't see what all we gain by adding this as there is no standard saying you have to have a version command. Most people know to add -v or --version and that works fine so I don't know why we need this…

As I pointed out in the initial comment, some other tools support --version, some support version, and some support both. I think the best place to be from a usability standpoint is supporting both.

I've pushed e09dd4f → 0865185 rebasing onto master and addressing your runC → runc and os.Stdout.WriteString → something we already use requests. Outstanding requests from your earlier review:

Let me know how you want those handled and I'll get them cleaned up too.

Signed-off-by: W. Trevor King <[email protected]>

AkihiroSuda · 2020-03-05T07:52:56Z

Can we close?

mikebrow · 2020-03-05T16:44:38Z

There is no requirement for a runtime version command in the runtime spec, the discussion linked above around adding one was closed.

The output for runc version information wrt. prefixes used is slightly different between -v and -help/usage output, but such differences are common.

For lack of a treatise on the subject the discussion in the spec led to a suggestion for leaning on GNU's standard:
https://www.gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html
https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion

But, as stated, no formal requirements were made in the runtime spec. So, IMO this should be closed and if there is interest in normalizing "version" usage/output we could/should take it up again at the spec level.

GordonTheTurtle added the status/0-triage label Jul 11, 2016

wking mentioned this pull request Jul 11, 2016

Add Runtime CLI Spec opencontainers/runtime-spec#513

Closed

mlaventure reviewed Jul 11, 2016
View reviewed changes

wking force-pushed the version-command branch from fbc7490 to b8ad6a4 Compare July 11, 2016 19:00

wking force-pushed the version-command branch from b8ad6a4 to 8ab786f Compare July 15, 2016 17:22

crosbymichael reviewed Aug 23, 2016
View reviewed changes

wking force-pushed the version-command branch 2 times, most recently from 79f908e to d99746a Compare August 23, 2016 18:30

wking force-pushed the version-command branch from d99746a to e09dd4f Compare September 8, 2016 05:28

crosbymichael requested changes Oct 31, 2016

View reviewed changes

wking force-pushed the version-command branch from e09dd4f to 0865185 Compare March 21, 2017 23:12

*: Add man page and Bash completion for 'runc version'

7698127

Signed-off-by: W. Trevor King <[email protected]>

Shay-am-Collab approved these changes Dec 24, 2019

View reviewed changes

AkihiroSuda closed this Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

version: Add a version command #940

version: Add a version command #940

wking commented Jul 11, 2016

mlaventure Jul 11, 2016 •

edited

Loading

mikebrow commented Jul 11, 2016

wking commented Jul 11, 2016

hqhq commented Jul 15, 2016

wking commented Jul 15, 2016

crosbymichael Aug 23, 2016

wking Aug 23, 2016

crosbymichael Aug 23, 2016

wking Aug 23, 2016

mikebrow Sep 13, 2016 •

edited

Loading

crosbymichael commented Aug 23, 2016

wking commented Aug 23, 2016

crosbymichael commented Aug 23, 2016

wking commented Aug 23, 2016

mikebrow commented Aug 23, 2016

wking commented Sep 8, 2016

crosbymichael left a comment

crosbymichael Oct 31, 2016

wking Mar 21, 2017

crosbymichael Oct 31, 2016

wking Mar 21, 2017 •

edited

Loading

crosbymichael Oct 31, 2016

wking Mar 21, 2017

wking commented Mar 21, 2017

AkihiroSuda commented Mar 5, 2020

mikebrow commented Mar 5, 2020

version: Add a version command #940

version: Add a version command #940

Conversation

wking commented Jul 11, 2016

mlaventure Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

mikebrow commented Jul 11, 2016

wking commented Jul 11, 2016

hqhq commented Jul 15, 2016

wking commented Jul 15, 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

mikebrow Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

crosbymichael commented Aug 23, 2016

wking commented Aug 23, 2016

crosbymichael commented Aug 23, 2016

wking commented Aug 23, 2016

mikebrow commented Aug 23, 2016

wking commented Sep 8, 2016

crosbymichael left a 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

wking Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Mar 21, 2017

AkihiroSuda commented Mar 5, 2020

mikebrow commented Mar 5, 2020

mlaventure Jul 11, 2016 •

edited

Loading

mikebrow Sep 13, 2016 •

edited

Loading

wking Mar 21, 2017 •

edited

Loading