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

build: Fix manpage generation #3729

Merged
merged 1 commit into from Oct 8, 2019
Merged

Conversation

seemethere
Copy link
Contributor

@seemethere seemethere commented Oct 8, 2019

Seems to be that docs/man/ctr.1.md and docs/man/containerd.1.md were
removed in #3637 and were not updated correctly in the Makefile, leading
to build failures like:

+ make man

make: *** No rule to make target `man/ctr.1', needed by `man'.  Stop.

Changes the gen-manpages command to be specific on which manpages are to
be generated.

Also added a check in the travis yml so that this gets checked on PRs

Signed-off-by: Eli Uriegas eli.uriegas@docker.com

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 8, 2019

Build succeeded.

@@ -75,7 +75,7 @@ script:
- travis_wait ../project/script/validate/vendor
- GOOS=linux script/setup/install-dev-tools
- go build -i .
- make check
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this needs to be skipped for Darwin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to do lower on, and with a check for TRAVIS_GOOS=linux

@seemethere seemethere force-pushed the fix_man branch 2 times, most recently from 6c73bba to c53d896 Compare October 8, 2019 04:58
@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 8, 2019

Build succeeded.

@codecov-io
Copy link

codecov-io commented Oct 8, 2019

Codecov Report

Merging #3729 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3729   +/-   ##
=======================================
  Coverage   42.13%   42.13%           
=======================================
  Files         131      131           
  Lines       14474    14474           
=======================================
  Hits         6099     6099           
  Misses       7467     7467           
  Partials      908      908
Flag Coverage Δ
#linux 45.58% <ø> (ø) ⬆️
#windows 37.16% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0c6b51...036db34. Read the comment docs.

Makefile Outdated
@@ -82,7 +82,7 @@ TEST_REQUIRES_ROOT_PACKAGES=$(filter \

# Project binaries.
COMMANDS=ctr containerd containerd-stress
MANPAGES=ctr.1 containerd.1 containerd-config.1 containerd-config.toml.5
MANPAGES=containerd-config.1 containerd-config.toml.5
Copy link
Member

Choose a reason for hiding this comment

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

if these two generated manpages aren't in the $MANPAGES list, then the install-man target will not install them to $(DESTDIR)/man. Can probably solve with some other target that looks for content in man/ but will need to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-added those manpages back to this MANPAGES variable.

Seems to be that docs/man/ctr.1.md and docs/man/containerd.1.md were
removed in containerd#3637 and were not updated correctly in the Makefile, leading
to build failures like:

    + make man

    make: *** No rule to make target `man/ctr.1', needed by `man'.  Stop.

Changes the gen-manpages command to be specific on which manpages are to
be generated.

Signed-off-by: Eli Uriegas <eli.uriegas@docker.com>
@seemethere
Copy link
Contributor Author

Ended up changing gen-manpages to have to be specific on which app is being generated so that we could included targets like:

man/containerd.1: FORCE
	@echo "$(WHALE) $@"
	go run cmd/gen-manpages/main.go containerd man/

man/ctr.1: FORCE
	@echo "$(WHALE) $@"
	go run cmd/gen-manpages/main.go ctr man/

Which will basically make it so that none of the other targets actually need changing.

The genman target was preserved with the same behavior but I'm not entirely sure it's actually needed anymore.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 8, 2019

Build succeeded.

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

Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan merged commit 772aaf1 into containerd:master Oct 8, 2019
@seemethere seemethere deleted the fix_man branch October 10, 2019 04:38
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

4 participants