Skip to content
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

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

Merged
merged 1 commit into from Jun 1, 2022

Conversation

thaJeztah
Copy link
Member

(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

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

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 <github@gone.nl>
@thaJeztah thaJeztah marked this pull request as ready for review May 27, 2022 17:57
Copy link
Member

@kzys kzys left a comment

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
Member Author

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
Member Author

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

Copy link
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants