Build: fix output directory when specified with O=

[fredrikekre]

Fix abspath call when output directory is specified with O= and SRCDIR is not set.

[staticfloat]

@fredrikekre does this at least fix the issue you were seeing?

[fredrikekre]

Yes.

[vtjnash]

How would SRCDIR manage not to get set? This will not do the same task.

[staticfloat]

SRCDIR gets set much later. This happens as one of the first things in the Makefile. Can you explain in what situation this will not do the same task?

[vtjnash]

SRCDIR is the first line of the Makefiles—it always gets set first. This won't do the same task in any situation, since it's computing a function of a different value.

[staticfloat]

SRCDIR is not defined within Make.inc; so its usage here is wrong. It will always expand to an empty string, due to the usage of :=. There is no situation in which this line of code will be correct; so we either need to define a value for SRCDIR earlier, remove the usage of :=, or manually insert something that will compute the value that SRCDIR "should" have had, which is what this PR does.

[vtjnash]

SRCDIR is not defined within Make.inc; so its usage here is wrong. It will always expand to an empty string, due to the usage of :=. There is no situation in which this line of code will be correct

Make.inc doesn't define any rules and is never used, so it should never expand to the empty string

or manually insert something that will compute the value that SRCDIR "should" have had, which is what this PR does.

That is not what this PR does. If it was that easy to compute, we would have done that originally.

[staticfloat]

I think what you're trying to say is that SRCDIR should be set in all calling Makefiles before including Make.inc, and it should therefore be set by the time we get to this line in Make.inc. In that case, could you explain what the proper value to set SRCDIR is for the top-level Makefile, because there is no SRCDIR set in that Makefile.

[fredrikekre]

Right, defining SRCDIR in the main Makefile, before

julia/Makefile

Line 2 in eddcbd7

include $(JULIAHOME)/Make.inc

will probably also fix the issue.

Edit: It does fix the problem with this diff:

diff --git a/Makefile b/Makefile
index ea67778..df9eb03 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,5 @@
 JULIAHOME := $(abspath $(dir $(lastword $(MAKEFILE_LIST))))
+SRCDIR := $(JULIAHOME)
 include $(JULIAHOME)/Make.inc
 
 VERSDIR := v`cut -d. -f1-2 < $(JULIAHOME)/VERSION`

[fredrikekre]

Demonstration of the problem:

[~/sandbox]$ git clone https://github.com/JuliaLang/julia.git julia-source
[~/sandbox]$ export SOURCE=$PWD/julia-source
[~/sandbox]$ export INSTALL=$PWD/julia-install
[~/sandbox]$ make -C $SOURCE O=$INSTALL print-BUILDDIR
make: Entering directory '/home/fredrik/sandbox/julia-source'
BUILDDIR=/home/fredrik/sandbox/julia-install/home/fredrik/sandbox/julia-source <-- ????
make: Leaving directory '/home/fredrik/sandbox/julia-source'

[fredrikekre]

FWIW, there also seems to be some ordering problem:

[ekref@vera2:~]$ make -C $SOURCE O=$INSTALL -j20
Building into /c3se/users/ekref/Vera/julia
find: ‘/c3se/users/ekref/Vera/julia/base’: No such file or directory
find: ‘/c3se/users/ekref/Vera/julia/doc’: No such file or directory
find: ‘/c3se/users/ekref/Vera/julia/base’: No such file or directory
find: ‘/c3se/users/ekref/Vera/julia/usr/share/julia/stdlib/v1.2/*/src’: No such file or directory
make: Entering directory `/c3se/users/ekref/Vera/julia-source'
make: Circular /c3se/users/ekref/Vera/julia/usr/bin <- /c3se/users/ekref/Vera/julia/usr/bin dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/etc/julia <- /c3se/users/ekref/Vera/julia/usr/etc/julia dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/include <- /c3se/users/ekref/Vera/julia/usr/include dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/include/julia <- /c3se/users/ekref/Vera/julia/usr/include/julia dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/lib <- /c3se/users/ekref/Vera/julia/usr/lib dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/lib/julia <- /c3se/users/ekref/Vera/julia/usr/lib/julia dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/libexec <- /c3se/users/ekref/Vera/julia/usr/libexec dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/share/julia <- /c3se/users/ekref/Vera/julia/usr/share/julia dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/share/julia/stdlib <- /c3se/users/ekref/Vera/julia/usr/share/julia/stdlib dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/share/man/man1 <- /c3se/users/ekref/Vera/julia/usr/share/man/man1 dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/tools <- /c3se/users/ekref/Vera/julia/usr/tools dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia <- /c3se/users/ekref/Vera/julia dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/base <- /c3se/users/ekref/Vera/julia/base dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/src <- /c3se/users/ekref/Vera/julia/src dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/ui <- /c3se/users/ekref/Vera/julia/ui dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/doc <- /c3se/users/ekref/Vera/julia/doc dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/deps <- /c3se/users/ekref/Vera/julia/deps dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/stdlib <- /c3se/users/ekref/Vera/julia/stdlib dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/test <- /c3se/users/ekref/Vera/julia/test dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/test/embedding <- /c3se/users/ekref/Vera/julia/test/embedding dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/test/llvmpasses <- /c3se/users/ekref/Vera/julia/test/llvmpasses dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/share/julia/base <- /c3se/users/ekref/Vera/julia/usr/share/julia/base dependency dropped.
make: Circular /c3se/users/ekref/Vera/julia/usr/share/julia/test <- /c3se/users/ekref/Vera/julia/usr/share/julia/test dependency dropped.
Creating usr/etc/julia/startup.jl
Copying in usr/share/man/man1/julia.1
/c3se/users/ekref/Vera/julia-source/contrib/install.sh 755 /c3se/users/ekref/Vera/julia-source/contrib/julia-config.jl /c3se/users/ekref/Vera/julia/usr/share/julia/
make[1]: Entering directory `/c3se/users/ekref/Vera/julia/stdlib'
make[1]: *** No targets specified and no makefile found.  Stop.
make[1]: Leaving directory `/c3se/users/ekref/Vera/julia/stdlib'
make: *** [julia-stdlib] Error 2
make: *** Waiting for unfinished jobs....
make: Leaving directory `/c3se/users/ekref/Vera/julia-source'

which is weird since julia-stdlib depends on DIRS

julia/Makefile

Line 59 in eddcbd7

julia-stdlib: | $(DIRS)

which includes the missing directories, like stdlib. I guess the unfinished jobs from the end of the output is creating those files, because calling make again works, since those file are now existing.

[JeffBezanson]

Should this be backported?

[fredrikekre]

Would #30431 (comment) be a better fix than what is on this branch now @vtjnash ?

[vtjnash]

That's at least not an incorrect change (unlike this PR), although nobody has yet said why this is a bug. There's a repro above that shows that an ignored variable has an undefined value—but which is normal to not care what happens to the value of something that is not used.

[fredrikekre]

nobody has yet said why this is a bug

You don't think this is a bug: #30431 (comment) ? I would expect

BUILDDIR=/home/fredrik/sandbox/julia-install

and not

BUILDDIR=/home/fredrik/sandbox/julia-install/home/fredrik/sandbox/julia-source

[Keno]

@vtjnash says this change is incorrect. It seems like there's still some confusion about that, so ideally let's figure out what the actual issue here is before triaging whether to backport it.

[staticfloat]

@vtjnash can you explain a little more concretely why this is incorrect?

[vtjnash]

It replaces some code that does the desired thing with code that does something else. The only claim so far seems to be that an unused variable has an undefined value. But yeah, we have many of those.

[staticfloat]

So let's talk about the incorrect behavior that we see right now then. The fundamental issue is that running make -C $SOURCE O=$INSTALL print-BUILDDIR doesn't work properly due to SRCDIR not being set within the top-level Makefile. Perhaps the proper fix is as @fredrikekre said above, and to define SRCDIR := $(JULIAHOME) within our top-level Makefile.

[vtjnash]

OK, but this can't be the only unused variable (used by some other arbitrary build script) that has a random value. As you point out, SRCDIR and SRCCACHE also don't have the correct values

[staticfloat]

@fredrikekre I tried to re-create this and I couldn't; if you include the make configure step from the README (Which I don't see in your code traces above) does this work properly for you now?