Skip to content

Makefile: use urfave_cli_no_docs for binaries that don't need it#6998

Merged
estesp merged 1 commit intocontainerd:mainfrom
thaJeztah:urfave_cli_no_docs
Jun 1, 2022
Merged

Makefile: use urfave_cli_no_docs for binaries that don't need it#6998
estesp merged 1 commit intocontainerd:mainfrom
thaJeztah:urfave_cli_no_docs

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented May 27, 2022

(first commit is from #6997 - I'll rebase once that's merged)

We only need the ToMan() as part of the bin/gen-manpages binary, which
generates the man-pages; other binaries don't use this code, so we can
set the urfave_cli_no_docs build-tag to exclude cpuguy83/md2man and
russross/blackfriday (and other dependencies) from the binaries:

Before:

ls -lh bin
total 149M
-rwxr-xr-x 1 root root  49M May 27 10:12 containerd
-rwxr-xr-x 1 root root 6.1M May 27 10:13 containerd-shim
-rwxr-xr-x 1 root root 8.1M May 27 10:13 containerd-shim-runc-v1
-rwxr-xr-x 1 root root 8.2M May 27 10:13 containerd-shim-runc-v2
-rwxr-xr-x 1 root root  22M May 27 10:12 containerd-stress
-rwxr-xr-x 1 root root  26M May 27 10:11 ctr
-rwxr-xr-x 1 root root  30M May 27 10:14 gen-manpages

ls -l bin
total 151676
-rwxr-xr-x 1 root root 51280184 May 27 10:12 containerd
-rwxr-xr-x 1 root root  6332416 May 27 10:13 containerd-shim
-rwxr-xr-x 1 root root  8458240 May 27 10:13 containerd-shim-runc-v1
-rwxr-xr-x 1 root root  8536064 May 27 10:13 containerd-shim-runc-v2
-rwxr-xr-x 1 root root 22567160 May 27 10:12 containerd-stress
-rwxr-xr-x 1 root root 26873752 May 27 10:11 ctr
-rwxr-xr-x 1 root root 30508888 May 27 10:14 gen-manpages

After:

ls -lh bin
total 147M
-rwxr-xr-x 1 root root  49M May 27 10:26 containerd
-rwxr-xr-x 1 root root 6.1M May 27 10:26 containerd-shim
-rwxr-xr-x 1 root root 8.1M May 27 10:26 containerd-shim-runc-v1
-rwxr-xr-x 1 root root 8.2M May 27 10:26 containerd-shim-runc-v2
-rwxr-xr-x 1 root root  22M May 27 10:26 containerd-stress
-rwxr-xr-x 1 root root  26M May 27 10:26 ctr
-rwxr-xr-x 1 root root  30M May 27 10:27 gen-manpages

ls -l bin
total 149912
-rwxr-xr-x 1 root root 50930360 May 27 10:26 containerd
-rwxr-xr-x 1 root root  6332416 May 27 10:26 containerd-shim
-rwxr-xr-x 1 root root  8458240 May 27 10:26 containerd-shim-runc-v1
-rwxr-xr-x 1 root root  8536064 May 27 10:26 containerd-shim-runc-v2
-rwxr-xr-x 1 root root 22209144 May 27 10:26 containerd-stress
-rwxr-xr-x 1 root root 26523896 May 27 10:26 ctr
-rwxr-xr-x 1 root root 30508888 May 27 10:27 gen-manpages

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented May 27, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@thaJeztah thaJeztah force-pushed the urfave_cli_no_docs branch from 299e0e1 to e0768be Compare May 27, 2022 11:10
@thaJeztah thaJeztah mentioned this pull request May 27, 2022
Merged
@thaJeztah
Makefile: use urfave_cli_no_docs for binaries that don't need it
1a8024b 
We only need the `ToMan()` as part of the `bin/gen-manpages` binary, which
generates the man-pages; other binaries don't use this code, so we can
set the `urfave_cli_no_docs` build-tag to exclude `cpuguy83/md2man` and
`russross/blackfriday` (and other dependencies) from the binaries:

Before:

    ls -lh bin
    total 149M
    -rwxr-xr-x 1 root root  49M May 27 10:12 containerd
    -rwxr-xr-x 1 root root 6.1M May 27 10:13 containerd-shim
    -rwxr-xr-x 1 root root 8.1M May 27 10:13 containerd-shim-runc-v1
    -rwxr-xr-x 1 root root 8.2M May 27 10:13 containerd-shim-runc-v2
    -rwxr-xr-x 1 root root  22M May 27 10:12 containerd-stress
    -rwxr-xr-x 1 root root  26M May 27 10:11 ctr
    -rwxr-xr-x 1 root root  30M May 27 10:14 gen-manpages

    ls -l bin
    total 151676
    -rwxr-xr-x 1 root root 51280184 May 27 10:12 containerd
    -rwxr-xr-x 1 root root  6332416 May 27 10:13 containerd-shim
    -rwxr-xr-x 1 root root  8458240 May 27 10:13 containerd-shim-runc-v1
    -rwxr-xr-x 1 root root  8536064 May 27 10:13 containerd-shim-runc-v2
    -rwxr-xr-x 1 root root 22567160 May 27 10:12 containerd-stress
    -rwxr-xr-x 1 root root 26873752 May 27 10:11 ctr
    -rwxr-xr-x 1 root root 30508888 May 27 10:14 gen-manpages

After:

    ls -lh bin
    total 147M
    -rwxr-xr-x 1 root root  49M May 27 10:26 containerd
    -rwxr-xr-x 1 root root 6.1M May 27 10:26 containerd-shim
    -rwxr-xr-x 1 root root 8.1M May 27 10:26 containerd-shim-runc-v1
    -rwxr-xr-x 1 root root 8.2M May 27 10:26 containerd-shim-runc-v2
    -rwxr-xr-x 1 root root  22M May 27 10:26 containerd-stress
    -rwxr-xr-x 1 root root  26M May 27 10:26 ctr
    -rwxr-xr-x 1 root root  30M May 27 10:27 gen-manpages

    ls -l bin
    total 149912
    -rwxr-xr-x 1 root root 50930360 May 27 10:26 containerd
    -rwxr-xr-x 1 root root  6332416 May 27 10:26 containerd-shim
    -rwxr-xr-x 1 root root  8458240 May 27 10:26 containerd-shim-runc-v1
    -rwxr-xr-x 1 root root  8536064 May 27 10:26 containerd-shim-runc-v2
    -rwxr-xr-x 1 root root 22209144 May 27 10:26 containerd-stress
    -rwxr-xr-x 1 root root 26523896 May 27 10:26 ctr
    -rwxr-xr-x 1 root root 30508888 May 27 10:27 gen-manpages

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the urfave_cli_no_docs branch from e0768be to 1a8024b Compare May 27, 2022 17:57
@thaJeztah thaJeztah marked this pull request as ready for review May 27, 2022 17:57
kzys
kzys approved these changes May 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kzys kzys left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know I could specify build tags for dependencies... Is it namespaced?

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented May 27, 2022

Afaik, build-tags are propagated to everything used in the build (so definitely also something to take into account in case a custom build-tag "conflicts" with build-tags defined by a dependency)

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented May 30, 2022

@estesp ptal (I think I recall a comment from you once about the CLI always performing/loading some markdown code 😂 - I may mis-remember though)

estesp
estesp approved these changes Jun 1, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp merged commit dd9e6a7 into containerd:main Jun 1, 2022
@thaJeztah thaJeztah deleted the urfave_cli_no_docs branch June 1, 2022 10:58
@thaJeztah thaJeztah mentioned this pull request Dec 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kzys kzys kzys approved these changes

@samuelkarp samuelkarp samuelkarp approved these changes

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah @k8s-ci-robot @kzys @samuelkarp @estesp