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

make: build the docs subdir only from within src #1591

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@bagder
Member

bagder commented Jun 19, 2017

... and don't build at all in include

Prompted-by-work-by: Simon Warta
Ref: #1590

@bagder bagder added the build label Jun 19, 2017

@@ -35,8 +35,8 @@ HTMLPAGES = $(GENHTMLPAGES) index.html

# Build targets in this file (.) before cmdline-opts to ensure that
# the curl.1 rule below runs first
SUBDIRS = libcurl . cmdline-opts
DIST_SUBDIRS = $(SUBDIRS) examples
SUBDIRS = cmdline-opts

This comment has been minimized.

@webmaster128

webmaster128 Jun 19, 2017

Contributor

I think this change conflicts with the comment above. As far as I understand the comment and the $(abs_builddir)/curl.1: rule in the same file, we need . before cmdline-opts.

@@ -43,6 +43,8 @@ AM_CPPFLAGS = -I$(top_srcdir)/include \

bin_PROGRAMS = curl

SUBDIRS = ../docs

This comment has been minimized.

@webmaster128

webmaster128 Jun 19, 2017

Contributor

I did not find a reference stating if SUBDIRS works for external directories. If it does, great.

This comment has been minimized.

@bagder

bagder Jun 19, 2017

Member

Yeah, I'll restore that, thanks for pointing it out. That's part of the magic fix necessary to make things build on non-gnu makes on *BSDs...

This comment has been minimized.

@webmaster128

webmaster128 Jun 19, 2017

Contributor

Was this the answer to the SUBDIRS = ../docs comment? Or to the other one, which you fixed?

This comment has been minimized.

@bagder

bagder Jun 19, 2017

Member

That became really weird. My response was regarding the docs/Makefile.am comment. I think the ../docs approach should work...

@bagder bagder force-pushed the bagder/make-build-docs-from-src branch from d458a9d to fa39432 Jun 19, 2017

@curl curl deleted a comment from coveralls Jun 19, 2017

@curl curl deleted a comment from coveralls Jun 19, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 19, 2017

Hm, it didn't really work out that simple. It broke the build as seen in the CI. I had to put back the explicit $(MANPAGE): rule in src/Makefile.am, which makes the change less neat ... testing this out now.

@coveralls

This comment has been minimized.

coveralls commented Jun 19, 2017

Coverage Status

Coverage increased (+0.4%) to 73.834% when pulling fa39432 on bagder/make-build-docs-from-src into 8621b61 on master.

@webmaster128

This comment has been minimized.

Contributor

webmaster128 commented Jun 19, 2017

With the current diff, there are still two ways building curl.1: SUBDIRS and the $(MANPAGE): rule in src/Makefile.am.

To avoid having two ways to archive one goal, I tried to come up with an alternative implementation of the $(MANPAGE): rule that ensures curl.1 is rebuild on dependency changes but can only think of solutions requiring helper Makefiles :(

@dfandrich

This comment has been minimized.

Collaborator

dfandrich commented Jun 19, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 19, 2017

Yeah, that makes me nervous about this change. Mac OS has gnu make and we have no BSD machines in the CI setup...

@webmaster128

This comment has been minimized.

Contributor

webmaster128 commented Jun 20, 2017

I tried having a /src/hugehelp/Makefile.am building all the dependencies for tool_hugehelp.c, but in that place, I do not have paths to the curl.1 dependencies available ($(DPAGES) $(OTHERPAGES) Makefile.inc do not contain paths, only filenames). So this does not work as well.

Thus we can only build curl.1 from /docs/cmdline-opts and /src depends on /docs. Automake cannot model this, since /docs is not a subdir of /src.

Is this pre-build code generation of curl.1 a good idea at all? It somehow mixes maintainer jobs and user jobs. I think there are two clean ways to code generation: either generate at build time by the user or pre-generate for the user. The first way does not work if Perl should not be required. The other way means duplication in the git repo and a maintainer script to generate curl.1. If we have a CI script checking that all generated code is up-to-date, I think this is a good solution.

Assuming pre-build code generation is a good idea, as far as I can see, the only proper way is to move the command line tool documentation into /src/docs

@bagder

This comment has been minimized.

Member

bagder commented Jun 20, 2017

Is this pre-build code generation of curl.1 a good idea at all?

You mean if it is sensible to allow people to build curl from source without requiring perl? Or alternatively, rewrite gen.pl in another language. I think it is worth it mostly for the Windows people, as they're usually the ones least likely to have perl around already.

the only proper way is to move the command line tool documentation into /src/docs

How would that make things better? I don't have any formal objections to that if it means an improved build setup.

@webmaster128

This comment has been minimized.

Contributor

webmaster128 commented Jun 20, 2017

You mean if it is sensible to allow people to build curl from source without requiring perl?

I mean half-done code generation of curl.1, that is part of the main build system but ships a pre-build file curl.1 that is not built with the first make but built again when doing something like make && make clean && make. I think any file should either be shipped and fixed or be generated by the build system.

How would that make things better?

/src/Makefile.am gets SUBDIRS = docs and everything from /docs/cmdline-opts moves into /src/docs, causing cd src/docs && make to update /src/docs/curl.1. /src/docs/curl.1 is only updated when necessary and can be pre-generated in the tarball.

This way, the commandline tool is rebuild, if any of the .d files changes because automake's SUBDIRS allows a clean recursive dependency setup.

@bagder

This comment has been minimized.

Member

bagder commented Jun 20, 2017

Won't it still have exactly the same challenges as now, with a file that can't get updated if perl is missing and shouldn't get updated if newer than its dependencies and the problem with non-gnu make on in-tree vs out-of-tree builds?

I don't see why docs/ is better than ../docs for the Makefile.

@bagder bagder force-pushed the bagder/make-build-docs-from-src branch from fa39432 to f1ec13d Jun 20, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 20, 2017

repushed the same patch again to have it tested on the updated CI distcheck which also does an out-of-tree-build

@webmaster128

This comment has been minimized.

Contributor

webmaster128 commented Jun 20, 2017

I don't see why docs/ is better than ../docs for the Makefile.

From the CI failure log of the commit in this PR without the explicit rule, where curl.1 was not available when /src/tool_hugehelp.c was build I think, external dirs in SUBDIRS like ../docs do not work.

@bagder

This comment has been minimized.

Member

bagder commented Jun 20, 2017

external dirs in SUBDIRS like ../docs

The problem there is that we're solving the problem in two different Makefiles, it won't matter if the other makefile is in docs/ or in ../docs. Neither is "external", but both are separate from the one in src/.

If we would merge the generation of curl.1 and the generation hugehelp.c into the same makefile, things would be much cleaner!

@coveralls

This comment has been minimized.

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.01%) to 73.793% when pulling f1ec13d on bagder/make-build-docs-from-src into 0feb762 on master.

@webmaster128

This comment has been minimized.

Contributor

webmaster128 commented Jun 20, 2017

SUBDIRS does indeed work for external directories, such as ../docs

I was having another look into https://travis-ci.org/curl/curl/builds/244466563 (diff: 8621b61...d458a9d), which should work. It turns out that tool_hugehelp.c is built before any of the SUBDIRS because it is in BUILT_SOURCES.

When I now apply 8621b61...d458a9d and remove BUILT_SOURCES = tool_hugehelp.c from src/Makefile.am, my build works fine. This is supported by the documentation, stating:

Moreover, all built sources do not necessarily have to be listed in BUILT_SOURCES. For instance, a generated .c file doesn’t need to appear in BUILT_SOURCES (unless it is included by another source), because it’s a known dependency of the associated object.

@bagder bagder force-pushed the bagder/make-build-docs-from-src branch from f1ec13d to bcc1c42 Jun 21, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 21, 2017

Thanks! I committed your change now and pushed to verify the CI likes this.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.01%) to 73.781% when pulling bcc1c42 on bagder/make-build-docs-from-src into 60c655a on master.

@webmaster128

This comment has been minimized.

Contributor

webmaster128 commented Jun 21, 2017

LGTM. This now removes the second way of building curl.1, which was my main point in #1590, in a minimal way.

Regarding other discussion:

  • I like the idea of not requring perl for the build, but I don't think there is another scriping laguage to replace it. So curl.1 should be shipped. Maybe at some point, we can pre-generate and check-in curl.1 to keep code generation separate from the user's build system.
  • The make process in docs can be simplified a lot if we move docs/curl.1 to docs/cmdline-opts/curl.1, where all it's dependencies are. But this is an independent follow-up.
@bagder

This comment has been minimized.

Member

bagder commented Jun 21, 2017

Maybe at some point, we can pre-generate and check-in curl.1 to keep code generation separate from the user's build system

That's also complicated since we want users who download the tarball to be able to write patches, improve things and be able to regenerate the man page as well to see how their local changes end up looking...

The make process in docs can be simplified a lot if we move docs/curl.1 to docs/cmdline-opts/curl.1, where all it's dependencies are. But this is an independent follow-up.

Yeah, we might as well do that...

bagder added some commits Jun 26, 2017

make: build the docs subdir only from within src
... and don't build at all in include

Prompted-by-work-by: Simon Warta
Ref: #1590

@bagder bagder force-pushed the bagder/make-build-docs-from-src branch from bcc1c42 to de769c0 Jun 26, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 26, 2017

Now I've also verified that this PR builds fine on these setups using the default (non-GNU) make:

  • FreeBSD in-tree build
  • FreeBSD out-of-tree build
@coveralls

This comment has been minimized.

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.01%) to 73.823% when pulling de769c0 on bagder/make-build-docs-from-src into 922f800 on master.

@bagder bagder closed this in d24838d Jun 30, 2017

@bagder bagder deleted the bagder/make-build-docs-from-src branch Jun 30, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 30, 2017

Thanks for all the help on this @webmaster128!

rylwin added a commit to rylwin/curl that referenced this pull request Jul 7, 2017

bagder added a commit that referenced this pull request Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment