curv3d / curv

a language for making art using mathematics
Apache License 2.0
1.14k stars 73 forks source link

Improve Build Process for Maintainers #41

Closed s-ol closed 6 years ago

s-ol commented 6 years ago

Hey, I just created an AUR package for curv.

There are two problems with the current build process I had to work around:

The Makefile doesn't expose the CMAKE flags

To create a binary package the installation root directory has to be set to a folder that can be bundled to form the package later. This requires setting -DCMAKE_INSTALL_PREFIX, which can only be set as a command line flag and is not exposed from your Makefile.

There is the DESTDIR env variable of make install that I can set, but cmake per default still modifies the path to point to $DESTDIR/usr/local, while Arch for example doesn't allow installing binaries to /usr/local/bin (they should go to /usr/bin) - so DESTDIR is not enough.

This also brings up the question - what is the benefit of the master Makefile? From a maintenance POV it seems preferrable to stick with the standard cmake build workflow.

The CMakeLists.txt doesn't honor CMAKE_INSTALL_PREFIX

In your CMakeLists.txt you install() your language specs to hardcoded paths. These paths should be correctly generated using CMAKE_INSTALL_PREFIX and ideally DESTDIR. In the current version this breaks my build by trying to install to a location outside of the build sandbox.

For now I fixed both issues by ignoring your master-Makefile and just unrolling the cmake setup myself, as well as including a patch that removes the lines I linked above and just omits the language specs.

prepare() {
  cd "$_pkgname"
  git submodule update --init
  patch -Np1 -i "${srcdir}/remove_lang_file.patch"

  echo '#define CURV_VERSION "'`git describe --tags --always --dirty`'"' >,v
  if cmp -s ,v libcurv/version.h; then rm ,v; else mv ,v libcurv/version.h; fi

  mkdir -p release
  cd release
  cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX="${pkgdir}"/usr ..
}

build() {
  cd "$_pkgname"/release
  make
}

package() {
  cd "$_pkgname"/release
  make install
}
doug-moen commented 6 years ago

@s-ol asks "what is the benefit of the master Makefile?"

The purpose of the Makefile is to provide a simple high level build interface to end users, who may not be C++ developers or cmake experts.

It is easier to type make than to type

mkdir release
chdir release
cmake .. -DCMAKE_BUILD_TYPE=Release
make

I think it is reasonable to bypass the Makefile for building the Arch package.

But that means we need to fix the build system so that the rule for constructing libcurv/version.h is moved out of the Makefile and into cmake.

doug-moen commented 6 years ago

After some research, I don't see how to program cmake to generate libcurv/version.h. Suggestions from cmake experts would be appreciated.

I am aware of add_custom_command and configure_file. However, the problem is that the command that generates libcurv/version.h must be executed every time you do a build, and I don't know how to do that in cmake. The time stamp of libcurv/version.h is only updated if the version has actually changed, based on querying git. Only if the git version has changed will curv/version.cc be recompiled and the curv executable be rebuilt.

Assuming that cmake is simply not powerful enough to do what I want, then we need to change the Makefile so that you can specify an install prefix. That's probably easier than fighting with cmake.

mkeeter commented 6 years ago

@doug-moen You're in luck – I ran into the same issue for libfive, and documented the solution here.

doug-moen commented 6 years ago

Thanks @mkeeter!

doug-moen commented 6 years ago

My solution is just a few of lines of code. In CMakeLists.txt:

execute_process(COMMAND sh -c "${CMAKE_SOURCE_DIR}/cmake/make_version.sh"
    WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}")

In cmake/make_version.sh:

echo '#define CURV_VERSION "'`git describe --tags --always --dirty`'"' >,v
if cmp -s ,v libcurv/version.h; then rm ,v; else mv ,v libcurv/version.h; fi

This is cleaner than my previous solution, doing the work in the Makefile.

s-ol commented 6 years ago

@doug-moen for the second part I am not sure how to handle it 'correctly' - i can just tell you that the current process of glob'ing which directories exist doesn't work for distributable binary builds.

The difference between /usr/share and /usr/local/share shouldn't concern you, you should just install( DESTINATION share/...) to let it take care of the prefix for you. On my arch machine I only have a sourceview-3.0 directory.

doug-moen commented 6 years ago

@s-ol: I replaced the scary looking 6 lines of code for installing curv.lang with the 1 line of code that you suggested. I tested it on 2 platforms, and it works. The best code is the simplest code that works, so I'm happy with the change.

s-ol commented 6 years ago

Awesome! I will update my PKGBUILD later today and close this when everything goes smoothly :)

BTW, didn't have a lot of time to play with it yet but curv is awesome! I liked it a lot more than OpenSCAD. And it all compiles to shaders? Can it be generalized to accept uniforms as well? (to embed and use inside other applications etc)

s-ol commented 6 years ago

Works great!