adrian-thurston / colm

The Colm Programming Language
MIT License
164 stars 31 forks source link

Some build system fixes #138

Closed viccie30 closed 2 years ago

viccie30 commented 2 years ago

While trying to get colm to build and test out of its source tree, I encountered some issues. I've tried to split the changes into meaningful patches, many of which can be applied independently from each other (with a little work).

The first commit (882a94938aae254aac05e061043f4c06851ea7fc) replaces the current code detecting whether colm is run from its source directory with an explicit option specifying a build directory to locate the needed headers and library. It also links with libtool, preferring a static link against libcolm like it currently is.

The second commit (044bd51df6b7a87d97e6d5e12c940c7ec8492880) moves the generation of version.h from the configure script itself to config.status. This means config.status is able to recreate the full configuration state and that the header is updated correctly and automatically rebuilt if deleted.

The third commit (29879d74ae4ec35e452d9fd34d8718378f19a821) is a preparation for the fourth. It deletes a header that's been empty since 2010 and isn't installed.

The fourth commit (c411d029734122fb6de8be8326ba803438c775e0) moves the generation of the src/include/colm directory to config.status. The list of files is copied from RUNTIME_HDR in src/Makefile.am (https://github.com/adrian-thurston/colm/blob/c77aceb85139404a602fbd463f631235f99773d9/src/Makefile.am#L38-L41). Maybe this can be deduplicated?

The fifth commit (cb0dc1bcb82a9ff4e623a49d05a52ae1f63d0626) ensures that colm can find its source files while building outside of its source directory. make's VPATH mechanism and colm's include mechanism work together for that.

The sixth commit (5cf37adb380b4ddf819951913c4d3044071f7e6b) makes the test Makefiles link or copy the needed data for all tests if running outside of their source directories. It also adds the possibility to run the tests with make check.

The seventh commit (5f674757fc35708f7d1700a9fc37a7b77a1bf1ab) is a change of behavior for the build system. Until now all supporting programs and scripts for testing have been built in the all target. This commit moves them to the check target.

The eighth commit (7e3709188833f72ca7a0808942d3a9da6464675f) make sure that make clean removes what make has built (https://www.gnu.org/software/automake/manual/html_node/Clean.html). Files and links created by config.status are left in place, because automake automatically cleans those with distclean.

The ninth commit (bb487b1368b65a1f30fe3a59fd0c7d84bdcc483b) makes colm honor the standard CC and CFLAGS environment variables. The defaults are kept the same and CFLAGS from the environment come after the colm defaults, so they override the defaults.

The tenth commit (7b2524ec0c036a3aab8e97e77c8a94b3dc12fe82 ) changes the single PREFIX define in src/main.cc to two separate defines for includedir and libdir. Both can be set independently and should be honored.

adrian-thurston commented 2 years ago

@viccie30 this is great work, thank you for the contribution. Just trying it out now on ubuntu 20.04.3.

automake (GNU automake) 1.16.1
autoconf (GNU Autoconf) 2.69
libtool (GNU libtool) 2.4.6

I need to add a description to AC_DEFINE_UNQUOTED, otherwise I get a warning from ./autogen.sh and an error at make time.

AC_DEFINE_UNQUOTED([PUBDATE], ["$PUBDATE"], [Publication Date])
[thurston@linux1] colm: ./autogen.sh 
+ case `uname` in
++ uname
+ libtoolize --quiet --copy --force
+ aclocal
+ autoheader
autoheader: warning: missing template: PUBDATE
autoheader: Use AC_DEFINE([PUBDATE], [], [Description])
+ automake --foreign --add-missing
configure.ac:36: installing './compile'
configure.ac:26: installing './missing'
src/Makefile.am: installing './depcomp'
+ autoconf
[thurston@linux1] colm: make
Making all in src
make[1]: Entering directory '/home/thurston/devel/colm/src'
(CDPATH="${ZSH_VERSION+.}:" && cd .. && /bin/bash /home/thurston/devel/colm/missing autoheader)
autoheader: warning: missing template: PUBDATE
autoheader: Use AC_DEFINE([PUBDATE], [], [Description])
make[1]: *** [Makefile:717: config.h.in] Error 1
make[1]: Leaving directory '/home/thurston/devel/colm/src'
make: *** [Makefile:491: all-recursive] Error 1

The other issue I found is a make clean deletes some things it shouldn't, when in the source tree. Some of the existing cleans were already incorrect before this PR. I think I'll just merge this PR and then fix these two issues afterwards.

Thanks again, great stuff.

viccie30 commented 2 years ago

Thanks for the quick review and merge!

The missing description was a (misguided) attempt to get autoheader to exclude PUBDATE from config.h. I just rechecked and it doesn't work in autoconf 2.71 either, it just doesn't warn about it anymore.

make clean did indeed clean too much when building in-source, but with your fix it cleans too little out-of-source. I've opened a new pull request (#140) to fix it cleanly.