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

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

Closed
wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder 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
docs/Makefile.am Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 Compare June 19, 2017 11:51
@curl curl deleted a comment from coveralls Jun 19, 2017
@curl curl deleted a comment from coveralls Jun 19, 2017
@bagder
Copy link
Member Author

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
Copy link

Coverage Status

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

@webmaster128
Copy link
Contributor

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
Copy link
Contributor

dfandrich commented Jun 19, 2017 via email

@bagder
Copy link
Member Author

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
Copy link
Contributor

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

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
Copy link
Contributor

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

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 Compare June 20, 2017 09:23
@bagder
Copy link
Member Author

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
Copy link
Contributor

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

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
Copy link

Coverage Status

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

@webmaster128
Copy link
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 Compare June 21, 2017 08:49
@bagder
Copy link
Member Author

bagder commented Jun 21, 2017

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

@coveralls
Copy link

Coverage Status

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

@webmaster128
Copy link
Contributor

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

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...

... 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 Compare June 26, 2017 10:19
@bagder
Copy link
Member Author

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
Copy link

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 June 30, 2017 15:10
@bagder
Copy link
Member Author

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 pushed a commit that referenced this pull request Jul 10, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants