Can't Update Pegtl (maybe other TPLs) Without Complete Rebuild
rspavel opened this issue · 5 comments
After noticing that pegtl was apparently updated at some point and I am no longer able to build quinoa without updating TPL, I went to update TPL.
It looks like a combination of the forced install to quinoa-tpl/install and the lack of version checking for all but one of the libraries means that cmake is unable to function properly and detect changed sources.
This can likely be fixed by adding version checking for all TPLs, much as it was done with boost. This is something we want to do anyway as we probably require features from versions within the past few years and need to be aware of major API changes forcing us to use a legacy installation in the future.
This behavior is only documented for the recent-ish pegtl update but was noticed when using a subset of quinoa-tpl on the testbed application a few months ago, but this was written off to being solely due to the mandatory install and was thought fixed when quinoa-tpl's location relative to quinoa became slightly less hardcoded.
Please see pasted a subset of the terminal output showing this issue.
[rspavel@foo bld]$ make
[ 4%] Built target pugixml
[ 12%] Built target hypre
[ 16%] Built target boost
[ 20%] Built target hdf5
[ 28%] Built target h5part
[ 32%] Built target testu01
[ 36%] Built target numdiff
[ 44%] Built target netcdf
[ 52%] Built target trilinos
[ 60%] Built target random123
[ 68%] Built target tut
[ 76%] Built target pstreams
[ 84%] Built target cartesian_product
[ 92%] Built target pegtl
[100%] Built target rngsse2[rspavel@foo tpl]$ git pull
remote: Counting objects: 36, done.
remote: Total 36 (delta 9), reused 9 (delta 9), pack-reused 27
Unpacking objects: 100% (36/36), done.
From github.com:quinoacomputing/quinoa-tpl
f4a084e..f372475 master -> origin/master
- [new branch] root -> origin/root
Fetching submodule cmake
From github.com:quinoacomputing/cmake-modules
0b3f3e3..66a25a8 master -> origin/master- [new branch] root -> origin/root
Updating f4a084e..f372475
Fast-forward
.gitmodules | 2 +-
cmake | 2 +-
src/pegtl | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
[rspavel@foo tpl]$ git submodule update src/pegtl/
Submodule path 'src/pegtl': checked out '26df6aa2d6e582d098b85478a34d4487de8c82c5'[rspavel@foo bld]$ make
[ 4%] Built target pugixml
[ 12%] Built target hypre
[ 16%] Built target boost
[ 20%] Built target hdf5
[ 28%] Built target h5part
[ 32%] Built target testu01
[ 36%] Built target numdiff
[ 44%] Built target netcdf
[ 52%] Built target trilinos
[ 60%] Built target random123
[ 68%] Built target tut
[ 76%] Built target pstreams
[ 84%] Built target cartesian_product
[ 92%] Built target pegtl
[100%] Built target rngsse2[rspavel@foo bld]$ cmake ../
-- Found PEGTL: /home/rspavel/Repo/quinoa/tpl/install/gnu-x86_64/include/pegtl-- Will build the following 1 TPLs: hdf5
-- Configuring done
-- Generating done
-- Build files have been written to: /home/rspavel/Repo/quinoa/tpl/bld[rspavel@foo bld]$ make
[100%] Built target hdf5
As I will be spending the next 20-60 minutes rebuilding the TPLs yet again so I can verify a bugfix at the quinoa level, this is probably a high priority issue.
Versioning of TPLs and being able to detect those versions from our cmake would be awesome. We don't don't any of this, so an update to any of TPLs currently requires a rebuild (and cmake does not detect it). Sorry. AFAIK, e.g., PEGTL does not (yet) support it and I have no idea about the other TPLs, some probably do.
I assume detecting TPL versions should be based on reading some header file from the TPL from cmake. If so, that requires changes to those TPLs that don't support version yet, which may be problematic and slow. But those that do we should write cmake code to detect them. Also is there any other way this can be done?
Actually, PEGTL does, to a degree. Looks like they have a version.hpp https://github.com/taocpp/PEGTL/blob/e43e0d1169bdb9b74b32318951db7fdce8a53b4d/include/tao/pegtl/version.hpp that was updated about 3 months back. Sub-optimal, but based on your commit message it looks like that would have done the trick
And I would say that any actively developed code should have a way to check the version (... do we?), and if it isn't actively developed we should actively be trying to find an alternative from a security vulnerability standpoint as this code is still agile enough that we can migrate to supported libraries.
This is a problem that is going to keep coming back to bite us (it has already bitten me probably 5 times within the past year...), so even if checking the version slows down our configuration step a bit I think we would benefit drastically from having a build system that doesn't require users to take a long coffee break every time we decide to break backwards compatibility and require an update.
Or, more pointedly, this lets users know what we actually require without relying on trial and error (including that, I think this probably bit me at least 20 times in the past year) or digging in to our code and figuring out what our minimum required version is (what I did to determine the boost version).
Nice find on PEGTL! (Last time I have communicated with @d-frey, they were only thinking about how to do versioning), so yes, it would be awesome to do version checking not only for PEGTL for all of our TPLs, and in general everything you say above, I could not agree more with it.
We (the authors of the PEGTL) are new to CMake, so I'm not sure if what we are doing is the right way. We've added a version descriptor to the CMakeLists.txt we provide as well as a header to the PEGTL itself which provides the version number (major, minor, patch).
We are also aware that this is not enough for a C++ header library if you are using our library within another library, especially when it's a header-only library itself (or at least the PEGTL is included in the other library's header files).
I have added some paragraphs to our documentation which describes two ways to handle the situation, please check Embedding the PEGTL.
If you know of any way to further improve our CMakeLists.txt or anything else, please let us know. Thanks. :)
@d-frey I think from your end things are pretty much fine. Depending on the nature of a fix we could run into a problem where we need a fix but the version number hasn't been updated, but that is going to be a special case either way.
I think we just need to update our FindPEGTL.cmake to actually check the version number, which is something we should have done from the start anyway.