Remove internal phony target

[makenowjust]

GNU Makefile's manual says:

A phony target should not be a prerequisite of a real target file

For instance it causes some problem:

$ make spec
build...
run...
$ make spec
build... ← ??? why rebuild? no file is edited.
run...

This pull request fixes it also.

[asterite]

The problem is that macros can change the resulting executable. This is specially true with the compiler, where the date changes each time you compile. I think this was the reason we wanted to have phony targets.

I actually don't know what "phony target" means, but in short when we do make crystal we want to compiler the compiler each time, regardless of whether files were changed. The compiler itself already takes care to do some basic incremental compilation, at least with object files.

[makenowjust]

@asterite

I think this was the reason we wanted to have phony targets.

No, I don't think it. Because this behavior comes from crystal and some targets depend deps target, such a line:

$(O)/crystal: deps $(SOURCES)

and deps is phony target. A phony target has no time stamp, so $(O)/crystal builds every time. It is just an accident (a.k.a bug). If we intend this behavior really, we could make $(O)/crystal a phony target.

Building the compiler is heavy process, and also building the spec is more heavy now. I want to skip it if possible, and Makefile has the ability to do it. When we want to re-build with no file change, we can use traditional touch command.

[asterite]

To be honest, I don't know much about Makefiles... 😊

So if others that do know about it (for example @waj, @jhass and @ysbaddaden) think this change is fine, than 👍 from me too.

[samueleaton]

@asterite Can I just say that I really respect your humility. 👌 Its actually a somewhat inspiring, thank you!

[makenowjust]

@asterite please...

[asterite]

Unfortunately I don't have experience with Makefiles so someone else should take care of this. Sorry 😊

[luislavena]

Hello,

I've a bit of experience with Makefiles and can confirm @makenowjust statements.

A phony dependency is good for tasks that produce no artifacts, but using that task as dependency of another that does produce them results in constant re-execution of it.

Even if the compiler can perform some incremental checks, the main semantics steps consumes a lot of time when processing the compiler.

Moving LLVM_EXT_OBJ and LIB_CRYSTAL_TARGET to be file dependencies will reduce that recompile phase.

With this change, the compiler or the spec files will only be re-compiled when there is actually a change in the code, either the source files or the LLVM extension object or the produced lib file.

Cheers.

[makenowjust]

@asterite @luislavena Thanks!!

[luislavena]

Great! Thank you @makenowjust

$ make clean
Using /usr/bin/llvm-config-3.6 [version=3.6.2]
rm -rf .build
rm -rf ./doc
rm -rf src/llvm/ext/llvm_ext.o
rm -rf src/ext/sigfault.o src/ext/libcrystal.a

$ make crystal
Using /usr/bin/llvm-config-3.6 [version=3.6.2]
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc `/usr/bin/llvm-config-3.6 --cxxflags`
cc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
CRYSTAL_CONFIG_PATH=`pwd`/src ./bin/crystal build  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib

$ make crystal
Using /usr/bin/llvm-config-3.6 [version=3.6.2]
make: Nothing to be done for 'crystal'.

$ touch src/compiler/crystal.cr

$ make crystal
Using /usr/bin/llvm-config-3.6 [version=3.6.2]
CRYSTAL_CONFIG_PATH=`pwd`/src ./bin/crystal build  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using compiled compiler at .build/crystal

😄

[asterite]

Now I can't use the compiler anymore :-(

$ make clean
Using /usr/local/bin/llvm-config [version=3.9.1]
rm -rf .build
rm -rf ./doc
rm -rf src/llvm/ext/llvm_ext.o
rm -rf src/ext/sigfault.o src/ext/libcrystal.a
$ make deps
Using /usr/local/bin/llvm-config [version=3.9.1]
make: *** No rule to make target `deps'.  Stop.
$ bin/crystal spec spec/std/int_spec.cr 
clang: error: no such file or directory: '/Users/asterite-manas/Projects/crystal/src/ext/libcrystal.a'
Error: execution of command failed with code: 1: `cc -o "/Users/asterite-manas/.cache/crystal/crystal-run-spec.tmp" "${@}"  -rdynamic  -lgmp -lxml2 -lpcre -lgc -lpthread /Users/asterite-manas/Projects/crystal/src/ext/libcrystal.a -levent -liconv -ldl -L/usr/lib -L/usr/local/lib`

Maybe something is missing in the Makefile for the deps target?

[asterite]

Hmm... why was deps removed?

My workflow is, for example:

make clean deps
bin/crystal spec spec/std/int_spec.cr

I don't always go through Makefile to do everything. Most of the time I test individual files, not the whole compiler...

Do you think it would be hard to get the deps target back? Otherwise I'll probably revert this commit.

[asterite]

Hmm... I think I'll just revert this. I still can't understand the point of why would one run make spec twice without a change in the middle. If I do make spec is because I want to run the specs, and I want that because I made changes. If I don't make changes I don't run make spec :-)

[makenowjust]

@asterite Please wait 5 mins! I fix deps target is back. This fix is just correct for me and @luislavena at least, I don't know you use deps target many times, sorry.

[asterite]

@makenowjust No problem at all, take your time :-)

[luislavena]

@asterite definitely wasn't aware that deps was used that much, as my regular workflow just involved what I've typed before, so please accept my apologies for not noticing the removal of that task.