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

Empty manpage over parallel build #12

Closed
eribertomota opened this issue Jan 12, 2023 · 12 comments
Closed

Empty manpage over parallel build #12

eribertomota opened this issue Jan 12, 2023 · 12 comments

Comments

@eribertomota
Copy link
Contributor

Hi @minfrin,

The last version of retry (1.0.5) fails to dynamically generate the manpage on Debian because, by default, Debian uses parallel build (-j option for make command). See an example below:

  dh_auto_build
	make -j2
make[1]: Entering directory '/builds/eriberto/retry/debian/output/source_dir'
make  all-am
make[2]: Entering directory '/builds/eriberto/retry/debian/output/source_dir'
gcc -DHAVE_CONFIG_H -I.   -Wdate-time -D_FORTIFY_SOURCE=2  -g -O2 -ffile-prefix-map=/builds/eriberto/retry/debian/output/source_dir=. -fstack-protector-strong -Wformat -Werror=format-security -c -o retry.o retry.c
which txt2man && ./retry --help | txt2man -s 1 -t "retry" -r "retry-1.0.5" > retry.1 || true
/usr/bin/txt2man
/bin/bash: line 1: ./retry: No such file or directory
/bin/bash ./libtool  --tag=CC   --mode=link gcc  -g -O2 -ffile-prefix-map=/builds/eriberto/retry/debian/output/source_dir=. -fstack-protector-strong -Wformat -Werror=format-security  -Wl,-z,relro -Wl,-z,now -o retry retry.o  
libtool: link: gcc -g -O2 -ffile-prefix-map=/builds/eriberto/retry/debian/output/source_dir=. -fstack-protector-strong -Wformat -Werror=format-security -Wl,-z -Wl,relro -Wl,-z -Wl,now -o retry retry.o 
make[2]: Leaving directory '/builds/eriberto/retry/debian/output/source_dir'
make[1]: Leaving directory '/builds/eriberto/retry/debian/output/source_dir'

Consequently, the only content of the retry.1 is:

.\" Text automatically generated by txt2man
.TH retry  "1" "retry-1.0.5" ""

I will send a PR to add a CI test that includes a check of the manpage content. Currently, this CI test fails.

IMHO, it is not a good generate a manpage from each build. I suggest to release a manpage generated by hand inside of the tarball. I uploaded 1.0.5 to Debian and I am using the bootstrapped tarball available for download[1]. This tarball already has a manpage; I made a patch to exclude the end of the Makefile.am and to use the provided manpage[2].

[1] https://github.com/minfrin/retry/releases/download/retry-1.0.5/retry-1.0.5.tar.gz
[2] https://salsa.debian.org/debian/retry/-/blob/debian/master/debian/patches/010_disable-manpage-generation.patch

About my PR to create a CI test, note that was included "CLEANFILES = retry.1" to Makefile.am to allow the make command to execute a perfect clean before a new build because the manpage originally doesn't exist on source code.

Thanks for your work.

@minfrin
Copy link
Owner

minfrin commented Jan 12, 2023

I was trying to track down the 010_disable-manpage-generation.patch to see what it did so I could make whatever problem it was solving go away, but struggled to find it.

The man page is identical to retry --help, keeping the two pages the same manually is likely to be error prone.

In theory moving the manpage off into it's own target might work. In other words nothing is done for all, you would have to run "make man" or something along those lines to regenerate retry.1.

Would that work?

I'm trying to figure out a way to make sure cross compiling works.

@minfrin
Copy link
Owner

minfrin commented Jan 12, 2023

Something like this:

Index: Makefile.am
===================================================================
--- Makefile.am	(revision 136)
+++ Makefile.am	(working copy)
@@ -7,6 +7,6 @@
 EXTRA_DIST = retry.spec
 dist_man_MANS = retry.1
 
-retry.1: retry.c $(top_srcdir)/configure.ac
+man:
 	which txt2man && ./retry --help | txt2man -d 1 -t "${PACKAGE_NAME}" -r "${PACKAGE_NAME}-${PACKAGE_VERSION}" > retry.1 || true
 

@eribertomota
Copy link
Contributor Author

-retry.1: retry.c $(top_srcdir)/configure.ac
+man:

This only will create a new target for makefile. When running make command, this target will be executed.

I suggest to create an external script (out of makefile) to create the manpage. I do it in my projects. See examples in dcfldd[2] and scrot[2].

[1] https://github.com/resurrecting-open-source-projects/dcfldd/tree/master/man
[2] https://github.com/resurrecting-open-source-projects/scrot/tree/master/man

@eribertomota
Copy link
Contributor Author

In my first post I wrote: "Currently, this CI test fails.". Maybe this is unclear. The CI test is ok and can be merged (if you think this is a good idea). The test fails because the manpage is wrong when made via build process.

@minfrin
Copy link
Owner

minfrin commented Jan 13, 2023

This only will create a new target for makefile. When running make command, this target will be executed.

I suggest to create an external script (out of makefile) to create the manpage. I do it in my projects. See examples in dcfldd[2] and scrot[2].

[1] https://github.com/resurrecting-open-source-projects/dcfldd/tree/master/man [2] https://github.com/resurrecting-open-source-projects/scrot/tree/master/man

An external script and a makefile target are largely the same thing, neither ideal.

I went digging and figured out how to generate the man page extending the dist target. This means the man page will be generated when the tarball is created, but will not otherwise be touched at build time. This should solve your problem and remove the need for the patch:

Index: Makefile.am
===================================================================
--- Makefile.am	(revision 136)
+++ Makefile.am	(working copy)
@@ -7,6 +7,6 @@
 EXTRA_DIST = retry.spec
 dist_man_MANS = retry.1
 
-retry.1: retry.c $(top_srcdir)/configure.ac
-	which txt2man && ./retry --help | txt2man -d 1 -t "${PACKAGE_NAME}" -r "${PACKAGE_NAME}-${PACKAGE_VERSION}" > retry.1 || true
+dist-hook:
+	which txt2man && ./retry --help | txt2man -d 1 -t "${PACKAGE_NAME}" -r "${PACKAGE_NAME}-${PACKAGE_VERSION}" > $(distdir)/retry.1 || true
 

@eribertomota
Copy link
Contributor Author

In my tests it is OK for Debian (good!), but it will fail for the make dist (but this is a problem for the upstream side only, not affecting Debian packaging). Please, go ahead if you like this solution. Feel free for ask me for new tests.

@minfrin
Copy link
Owner

minfrin commented Jan 15, 2023

I've created a PR with the manpage having a proper target, and the dist hook depending on the manpage.

Can you see if this makes sense: #15

I can then pull in the tests.

@minfrin
Copy link
Owner

minfrin commented Jan 16, 2023

I'm getting closer.

The manpage build is forced at dist time, but is otherwise only rebuilt if missing. This means that as long as your deploy from a proper dist tarball, you will always get the same contents in the man page which remain unchanged.

I updated the tests in the dist branch to build a dist tarball, and then attempt a build from that, twice.

I have been struggling to get these two builds to be identical, it seems the RPATH buried in the binaries is different and refuses to be stripped. Stripping works on MacOS, but it doesn't appear to work in Ubuntu.

@minfrin
Copy link
Owner

minfrin commented Jan 16, 2023

If I set the SOURCE_DATE_EPOCH, will this give you a reproducible manpage build?

https://reproducible-builds.org/docs/source-date-epoch/

@eribertomota
Copy link
Contributor Author

On Debian systems, SOURCE_DATE_EPOCH is honored. My last changelog was made in 12 Jan 2023. The manpage is created using this date. IMO, the error is in txt2man command:

retry.1: retry
-	./retry --help | txt2man -d 1 -t "${PACKAGE_NAME}" -r "${PACKAGE_NAME}-${PACKAGE_VERSION}" > retry.1
+	./retry --help | txt2man -s 1 -t "${PACKAGE_NAME}" -r "${PACKAGE_NAME}-${PACKAGE_VERSION}" > retry.1

@minfrin
Copy link
Owner

minfrin commented Jan 17, 2023

I think I have it.

Building the tool twice results in identical binaries and manpages.

@minfrin
Copy link
Owner

minfrin commented Jan 22, 2023

In theory this should be fixed, and the manpage should "just work". Please reopen if I broke it.

@minfrin minfrin closed this as completed Jan 22, 2023
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

No branches or pull requests

2 participants