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

curl-7.54.0 introduced GNU make dependency #1432

Closed
0-wiz-0 opened this Issue Apr 19, 2017 · 19 comments

Comments

Projects
None yet
5 participants
@0-wiz-0

0-wiz-0 commented Apr 19, 2017

curl is usually buildable with BSD make, but in 7.54.0 it started using "$<" again, which in BSD make is only defined in implied rules, not in explicit ones.
Fix: in src/Makefile.am use $(MANPAGE) instead of $< (three places) and update the comment.

@bagder bagder added the build label Apr 19, 2017

@bagder bagder closed this in 5b4cbcf Apr 19, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 19, 2017

Member

thanks!

Member

bagder commented Apr 19, 2017

thanks!

@dfandrich

This comment has been minimized.

Show comment
Hide comment
@dfandrich

dfandrich Apr 19, 2017

Collaborator

This reverts b1dc45a which is going to break out-of-tree tar ball builds again.

Collaborator

dfandrich commented Apr 19, 2017

This reverts b1dc45a which is going to break out-of-tree tar ball builds again.

@dfandrich dfandrich reopened this Apr 19, 2017

@dfandrich

This comment has been minimized.

Show comment
Hide comment
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 20, 2017

Member

Tricky. Why does it break the out-of-tree build? I've successfully run my own test builds out-of-tree locally... Can you reproduce that error manually and see if you can understand it?

Or how exactly is $< different than $(MANPAGE) there that explains this?

Member

bagder commented Apr 20, 2017

Tricky. Why does it break the out-of-tree build? I've successfully run my own test builds out-of-tree locally... Can you reproduce that error manually and see if you can understand it?

Or how exactly is $< different than $(MANPAGE) there that explains this?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 20, 2017

Member

The problem is building from a tarball where there's a prebuilt docs/curl.1 already so the modified (commit 5b4cbcf) approach doesn't properly rebuild curl.1 in the build tree but still refers to the build tree path in the src/Makefile that builds the curl --manual feature...

Member

bagder commented Apr 20, 2017

The problem is building from a tarball where there's a prebuilt docs/curl.1 already so the modified (commit 5b4cbcf) approach doesn't properly rebuild curl.1 in the build tree but still refers to the build tree path in the src/Makefile that builds the curl --manual feature...

@ptor

This comment has been minimized.

Show comment
Hide comment
@ptor

ptor Apr 20, 2017

Or how exactly is $< different than $(MANPAGE) there that explains this?
I think it's because $< points to the prerequisite (that is, the dependency), which is searched via VPATH, while the $(MANPAGE) variable, when used directly in the rule, is not searched by VPATH - it's just replaced with its value.
A simple test:
--
$ cat Makefile
dep1 = dep1
dep2 = dep2
VPATH = dir
target: $(dep1) $(dep2)
@echo $< and $(dep1) and $(dep2)
--
$ mkdir dir
$ touch dir/dep1
$ touch dep2
--
$ make
dir/dep1 and dep1 and dep2

ptor commented Apr 20, 2017

Or how exactly is $< different than $(MANPAGE) there that explains this?
I think it's because $< points to the prerequisite (that is, the dependency), which is searched via VPATH, while the $(MANPAGE) variable, when used directly in the rule, is not searched by VPATH - it's just replaced with its value.
A simple test:
--
$ cat Makefile
dep1 = dep1
dep2 = dep2
VPATH = dir
target: $(dep1) $(dep2)
@echo $< and $(dep1) and $(dep2)
--
$ mkdir dir
$ touch dir/dep1
$ touch dep2
--
$ make
dir/dep1 and dep1 and dep2

@dfandrich

This comment has been minimized.

Show comment
Hide comment
@dfandrich

dfandrich Apr 20, 2017

Collaborator

It's only a problem on builds from the tar ball. The issue has to do with rebuilding the curl.1 file. In the tar ball, the time stamps are such that make does not actually rebuild the file, so the only copy is $(top_srcdir)/docs/curl.1. On git checkouts, the file must be rebuilt (because it's not available from the tar ball) so the only copy is in $(top_builddir)/docs/curl.1. $< uses the VPATH to find the right location of the file, whereas $(MANPAGE) hard-codes it. Yes, it's tricky and it took me a while to come up with b1dc45a. I thought I also verified that $< was standard make syntax before checking it in.

Collaborator

dfandrich commented Apr 20, 2017

It's only a problem on builds from the tar ball. The issue has to do with rebuilding the curl.1 file. In the tar ball, the time stamps are such that make does not actually rebuild the file, so the only copy is $(top_srcdir)/docs/curl.1. On git checkouts, the file must be rebuilt (because it's not available from the tar ball) so the only copy is in $(top_builddir)/docs/curl.1. $< uses the VPATH to find the right location of the file, whereas $(MANPAGE) hard-codes it. Yes, it's tricky and it took me a while to come up with b1dc45a. I thought I also verified that $< was standard make syntax before checking it in.

bagder added a commit that referenced this issue Apr 20, 2017

Revert "src/Makefile.am: avoid explicit $<"
This reverts commit 5b4cbcf.

Since it broke out-of-tree builds from tarballs. See discussion in #1432
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 20, 2017

Member

Ok, so let's start over. I've revered this change for now since it wasn't the right thing.

Member

bagder commented Apr 20, 2017

Ok, so let's start over. I've revered this change for now since it wasn't the right thing.

@dfandrich

This comment has been minimized.

Show comment
Hide comment
@dfandrich

dfandrich Apr 20, 2017

Collaborator

w.r.t. standard make syntax, $< is used all over the place in the curl automakefiles.

Collaborator

dfandrich commented Apr 20, 2017

w.r.t. standard make syntax, $< is used all over the place in the curl automakefiles.

@0-wiz-0

This comment has been minimized.

Show comment
Hide comment
@0-wiz-0

0-wiz-0 Apr 20, 2017

To clarify, $< is fine in implied rules, silly example:

.c.o:
    $(CC) -c $<

However, in this case it is an explicit rule:

foo.c:
    $(CC) -c $<

In this case, $< is not defined, at least in FreeBSD's and NetBSD's make.

0-wiz-0 commented Apr 20, 2017

To clarify, $< is fine in implied rules, silly example:

.c.o:
    $(CC) -c $<

However, in this case it is an explicit rule:

foo.c:
    $(CC) -c $<

In this case, $< is not defined, at least in FreeBSD's and NetBSD's make.

@dfandrich

This comment has been minimized.

Show comment
Hide comment
@dfandrich

dfandrich Apr 20, 2017

Collaborator

It's used at an explicit rule at least once, too: in the ca-bundle in the root Makefile.am

Collaborator

dfandrich commented Apr 20, 2017

It's used at an explicit rule at least once, too: in the ca-bundle in the root Makefile.am

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 20, 2017

Member

@0-wiz-0 so it evaluates completely empty then?

Member

bagder commented Apr 20, 2017

@0-wiz-0 so it evaluates completely empty then?

@0-wiz-0

This comment has been minimized.

Show comment
Hide comment
@0-wiz-0

0-wiz-0 Apr 20, 2017

ca-bundle: that needs fixing too, then.
$<: yes, it is empty in explicit rules.

0-wiz-0 commented Apr 20, 2017

ca-bundle: that needs fixing too, then.
$<: yes, it is empty in explicit rules.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 20, 2017

Member

The ca-bundle build can easily just use the variable instead, that's easy. The manpage thing however is much more complicated...

Member

bagder commented Apr 20, 2017

The ca-bundle build can easily just use the variable instead, that's easy. The manpage thing however is much more complicated...

@0-wiz-0

This comment has been minimized.

Show comment
Hide comment
@0-wiz-0

0-wiz-0 Apr 21, 2017

https://github.com/curl/curl/blob/master/src/Makefile.am gives me the following for the MANPAGE variable:

MANPAGE=$(top_builddir)/docs/curl.1

This is an absolute path, so VPATH does not play into this. What am I missing?

0-wiz-0 commented Apr 21, 2017

https://github.com/curl/curl/blob/master/src/Makefile.am gives me the following for the MANPAGE variable:

MANPAGE=$(top_builddir)/docs/curl.1

This is an absolute path, so VPATH does not play into this. What am I missing?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 21, 2017

Member

This is an absolute path, so VPATH does not play into this. What am I missing?

I'm not sure what you're missing but clearly the previously discussed fix was not good enough for out-of-tree builds. Do you have another suggested take on a fix?

Member

bagder commented Apr 21, 2017

This is an absolute path, so VPATH does not play into this. What am I missing?

I'm not sure what you're missing but clearly the previously discussed fix was not good enough for out-of-tree builds. Do you have another suggested take on a fix?

@aixtools

This comment has been minimized.

Show comment
Hide comment
@aixtools

aixtools Apr 21, 2017

Contributor

If I understand the viariables correctly $(top_build_dir) is where I ran configure from, where there is a different "top" dir (do not know auto-tools well enough), maybe $(top_src_dir).

One hack could be: check is $(top_src_dir) and $(top_builddir) are equal (during ./configure ??) and define $(MANPAGE) to point at $(top_src_dir) when coming from tarball and to $(top_builddir) when it is going to be generated.
In any case, the "old" way (v7.53.1 worked in any case) worked fine, from tarballs, "out-of-tree".

Update: Sorry - had not seen this was closed already. Ignore.

Contributor

aixtools commented Apr 21, 2017

If I understand the viariables correctly $(top_build_dir) is where I ran configure from, where there is a different "top" dir (do not know auto-tools well enough), maybe $(top_src_dir).

One hack could be: check is $(top_src_dir) and $(top_builddir) are equal (during ./configure ??) and define $(MANPAGE) to point at $(top_src_dir) when coming from tarball and to $(top_builddir) when it is going to be generated.
In any case, the "old" way (v7.53.1 worked in any case) worked fine, from tarballs, "out-of-tree".

Update: Sorry - had not seen this was closed already. Ignore.

bagder added a commit that referenced this issue Apr 21, 2017

build: avoid GNU-makeism for docs/curl.1 use
... but keep out-of-tree builds functional.

Fixes #1432
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 21, 2017

Member

I tested #1438 with both in-tree and out-of-tree builds and it seems to work as planned, and it doesn't use $< for this anymore.

Member

bagder commented Apr 21, 2017

I tested #1438 with both in-tree and out-of-tree builds and it seems to work as planned, and it doesn't use $< for this anymore.

bagder added a commit that referenced this issue Apr 21, 2017

build: avoid GNU-makeism for docs/curl.1 use
... but keep out-of-tree builds functional.

Fixes #1432
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 2, 2017

Member

This is now fixed after #1442 and follow-up commit.

Member

bagder commented May 2, 2017

This is now fixed after #1442 and follow-up commit.

@bagder bagder closed this May 2, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

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