strip path-info from `-v` (version) output, and implement -v flag for containerd-shim by thaJeztah · Pull Request #6495 · containerd/containerd

thaJeztah · 2022-01-31T12:04:57Z

runtime/v2/shim: strip path information from version output

I noticed that path information showed up in the version output:

./bin/containerd-shim-runc-v1 -v
./bin/containerd-shim-runc-v1:
Version:  v1.6.0-rc.1
Revision: ad77111.m
Go version: go1.17.2

POSIX guidelines describes; https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion

The program’s name should be a constant string; don’t compute it from argv[0].
The idea is to state the standard or canonical name for the program, not its
file name.

Unfortunately, this code is used by multiple binaries, so we can't fully remove the use of os.Args[0], but let's make a start and just remove the path info.

integration/images/volume-ownership: strip path information from usage output

POSIX guidelines describes; https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion

The program’s name should be a constant string; don’t compute it from argv[0].
The idea is to state the standard or canonical name for the program, not its
file name.

We don't have a const for this, but let's make a start and just remove the path info.

cmd/containerd-shim: add -v (version) flag

Unlike the other shims, containerd-shim did not have a -v (version) flag:

./bin/containerd-shim-runc-v1 -v
./bin/containerd-shim-runc-v1:
Version: v1.6.0-rc.1
Revision: ad77111.m
Go version: go1.17.2

./bin/containerd-shim -v
flag provided but not defined: -v
Usage of ./bin/containerd-shim:

This patch adds a -v flag to be consistent with the other shims. The code was
slightly refactored to match the implementation in the other shims, taking the
same approach as

containerd/runtime/v2/shim/shim.go

Lines 240 to 256 in 77d53d2

    
           func run(ctx context.Context, manager Manager, initFunc Init, name string, config Config) error { 
        
           	parseFlags() 
        
           	if versionFlag { 
        
           		fmt.Printf("%s:\n", os.Args[0]) 
        
           		fmt.Println("  Version: ", version.Version) 
        
           		fmt.Println("  Revision:", version.Revision) 
        
           		fmt.Println("  Go version:", version.GoVersion) 
        
           		fmt.Println("") 
        
           		return nil 
        
           	} 
        
           	if namespaceFlag == "" { 
        
           		return fmt.Errorf("shim namespace cannot be empty") 
        
           	} 
        
           	setRuntime()

kzys · 2022-01-31T19:50:37Z

integration/images/volume-ownership/tools/get_owner_windows.go

 func main() {
 	if len(os.Args) != 2 {
-		fmt.Printf("Usage: %s file_or_directory\n", os.Args[0])
+		fmt.Printf("Usage: %s file_or_directory\n", filepath.Base(os.Args[0]))


I didn't know that POSIX has recommendations around such a detail...

This one seems only used by get_owner_windows.exe. Should we use the name instead?

I didn't know that POSIX has recommendations around such a detail...

Yes, it's quite detailed, but also looks like nobody really follows the recommendations (including quite "posix-y" tools). That said; I thought it made sense to not make the output depend on "how" you call the binary (e.g. don't print ../../../../<binary> if you happen to use that as path 😂)

There were a couple more (somewhat interesting) recommendations in there, for example: https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion

the version number proper starts after the last space

(could be useful for easily parse / get the version from the output, especially given that we also output "git commit" etc.

Some other recommendations about licensing, and "support" / "project" URLs in output of --help (perhaps URL to where to report security issues etc?) https://www.gnu.org/prep/standards/html_node/_002d_002dhelp.html#g_t_002d_002dhelp

Anyway; not very urgent, but more of a "nice to do at some point"

This one seems only used by get_owner_windows.exe. Should we use the name instead?

oh, good on; I guess for this one, we could hard code it; let me change that

Well, I will dust off the rock I crawl under when I forget guidelines I should never forget. Thanks for taking care of this!

You're welcome 😅. Yeah (as outlined above); even though there's POSIX, it looks like nobody really follows the rules, so I consider it a "best effort" (and something to keep in the back of my mind when working on these things)

I noticed that path information showed up in the version output: ./bin/containerd-shim-runc-v1 -v ./bin/containerd-shim-runc-v1: Version: v1.6.0-rc.1 Revision: ad77111.m Go version: go1.17.2 POSIX guidelines describes; https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion > The program’s name should be a constant string; don’t compute it from argv[0]. > The idea is to state the standard or canonical name for the program, not its > file name. Unfortunately, this code is used by multiple binaries, so we can't fully remove the use of os.Args[0], but let's make a start and just remove the path info. Signed-off-by: Sebastiaan van Stijn <[email protected]>

…e output POSIX guidelines describes; https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion > The program’s name should be a constant string; don’t compute it from argv[0]. > The idea is to state the standard or canonical name for the program, not its > file name. We don't have a const for this, but let's make a start and just remove the path info. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2022-01-31T20:08:04Z

cmd/containerd-shim/main_unix.go

+func main() {
+	parseFlags()
+	if versionFlag {
+		fmt.Printf("%s:\n", filepath.Base(os.Args[0]))


Actually; this one is also only used for containerd-shim, so we can hard-code it here as well

Unlike the other shims, containerd-shim did not have a -v (version) flag: ./bin/containerd-shim-runc-v1 -v ./bin/containerd-shim-runc-v1: Version: v1.6.0-rc.1 Revision: ad77111.m Go version: go1.17.2 ./bin/containerd-shim -v flag provided but not defined: -v Usage of ./bin/containerd-shim: This patch adds a `-v` flag to be consistent with the other shims. The code was slightly refactored to match the implementation in the other shims, taking the same approach as https://github.com/containerd/containerd/blob/77d53d2d230c3bcd3f02e6f493019a72905c875b/runtime/v2/shim/shim.go#L240-L256 Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2022-01-31T20:10:38Z

Updated 👍 ptal

thaJeztah · 2022-02-08T09:00:38Z

@dmcgowan @estesp ptal; relatively small change (consistently having a -v flag on all binaries makes it easy to do a quick check if the binaries are "functional")

kzys · 2022-03-08T18:40:00Z

@estesp Looks good to go?

estesp

LGTM

kzys reviewed Jan 31, 2022

View reviewed changes

thaJeztah added 2 commits January 31, 2022 21:01

thaJeztah commented Jan 31, 2022

View reviewed changes

thaJeztah force-pushed the improve_versions branch from cbac8b8 to fdbfde5 Compare January 31, 2022 20:10

kzys approved these changes Feb 8, 2022

View reviewed changes

estesp approved these changes Mar 8, 2022

View reviewed changes

estesp merged commit b521429 into containerd:main Mar 8, 2022

thaJeztah deleted the improve_versions branch March 9, 2022 00:00

thaJeztah mentioned this pull request Aug 6, 2024

[master] verify: remove workaround for containerd-shim 1.6's missing "-v" flag docker/docker-ce-packaging#1044

Merged

thaJeztah mentioned this pull request Aug 15, 2024

Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION opencontainers/runc#4370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

strip path-info from `-v` (version) output, and implement -v flag for containerd-shim #6495

strip path-info from `-v` (version) output, and implement -v flag for containerd-shim #6495
estesp merged 3 commits intocontainerd:mainfrom
thaJeztah:improve_versions

thaJeztah commented Jan 31, 2022

Uh oh!

kzys Jan 31, 2022

Uh oh!

thaJeztah Jan 31, 2022

Uh oh!

gabriel-samfira Mar 10, 2022

Uh oh!

thaJeztah Mar 10, 2022

Uh oh!

thaJeztah Jan 31, 2022

Uh oh!

thaJeztah commented Jan 31, 2022

Uh oh!

thaJeztah commented Feb 8, 2022

Uh oh!

kzys commented Mar 8, 2022

Uh oh!

estesp left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	func run(ctx context.Context, manager Manager, initFunc Init, name string, config Config) error {
	parseFlags()
	if versionFlag {
	fmt.Printf("%s:\n", os.Args[0])
	fmt.Println(" Version: ", version.Version)
	fmt.Println(" Revision:", version.Revision)
	fmt.Println(" Go version:", version.GoVersion)
	fmt.Println("")
	return nil
	}

	if namespaceFlag == "" {
	return fmt.Errorf("shim namespace cannot be empty")
	}

	setRuntime()

Comments

Conversation

thaJeztah commented Jan 31, 2022

runtime/v2/shim: strip path information from version output

integration/images/volume-ownership: strip path information from usage output

cmd/containerd-shim: add -v (version) flag

Uh oh!

kzys Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

gabriel-samfira Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 31, 2022

Uh oh!

thaJeztah commented Feb 8, 2022

Uh oh!

kzys commented Mar 8, 2022

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants