Closed agatti closed 9 years ago
It seems like a good patch to me, and it works fine on an OSX environment. My guess is that this is harmless on a non-OSX environment as it will just fallback to the builtin find_package
macro.
The only question I have is, are we sure that we aren't breaking the way environment variables are read? This patch is pushing things on top of 4 CMake variables that are frequently set passing the -D flag to cmake: what happens if I want to use a bison version that I have installed outside the brew's path?
e.g.:
1) $ cmake -DCMAKE_INCLUDE_PATH=/some/path ..
at this point, the CMAKE_INCLUDE_PATH will be set to /some/path
;
2) Once the find_package_prefer_brew
macro will be called, will the highest priority be held by the environment or by the macro? Because to me, the macro is overriding the environment, as the CMAKE_INCLUDE_PATH
will then read /usr/local/include;/some/path
.
So, how about making the homebrew variables overriding automatic if and only if CMAKE_{MODULE,PROGRAM,INCLUDE,LIBRARY}_PATH are empty then? Maybe this can be restricted even further by applying an explicit check for OSX (well, darwin) and use plain find_package for Bison and Flex on anything that's not OSX.
I've made some minor changes to the script, hope these help solving the issues that have just been raised.
That's great. I have no possibility to test your code on Mac OS, but I will do some test builds on Windows and Linux to make sure nothing is broken. I agree with @massix in that we should not alter the environment in a way that could override user preferences, however I see you have fixed the bad bits.
In order to keep the build script clean, you should move your code to a separate file in cmake/
subdir and include
it in the main script. You should also close your blocks with endif()
and the like (without repeating the condition), to decrease verbosity.
There it is, hope it looks ok now. I didn't set a custom module path otherwise it would mess with values that might have been set by the user, so a full path to the homebrew support code was more or less in order (I'm not a CMake expert, after all).
Unfortunately I am quite busy right now, so it will take a bit before I can validate and finally merge the changes. In the meantime, if anyone is willing to give this changeset a try, I will welcome his/her feedback.
This simple change sort of works around the need of playing with environment variables to let CMake pick up a more recent version of Bison on OSX if one is available. Currently not tested on anything but OSX+Homebrew, you may want to see if it doesn't create problems on either Linux or Windows, though.