Cmake build #97

papadop · 2019-02-20T13:05:29Z

Here is a cmake build for blitz. Minimally tested on linux (but I have in the past compiled on MacOS and Windows, so I have good hopes). There are probably some polishing to be done (notably make test which does not build dependencies and the hao-he benchmark which does not compile due to the removal of vector.h), but most of the functionality is there.

To use it:

Go to a new directory.
cmake path-to-the-source-directory
make (or make lib)
make check-{testsuite,benchmarks,examples}

This PR also contains a trivial correction for iter.cpp (elapsedSeconds->elapsed) which was needed to make teh test pass. check-testsuite and check-examples pass. check-benchmarks currently fails due to the hao-he test. I do not know whether we have to correct the test or remove it. So I left it untouched for now.

citibeth · 2019-02-22T14:05:32Z

benchmarks/iter.cpp

@@ -19,7 +19,7 @@ int main() {
    }
    timer.stop();
    double flops = 10.0*512*512*512*2;
-    double seconds = timer.elapsedSeconds();
+    double seconds = timer.elapsed();


What's the reason for this? (I'm assuming that elapsed() is more standard and more likely to work on modern compilers?)

Well my compiler g++ 8.2.1 complained. I didi it mechanically to allow the test to compile. I must say I was even thinking it was a change internal to blitz (I though it had its own timer class)...

I see.... looks like it was just a bug, and nobody had bothered to compile benchmarks/iter.cpp, which depends on Blitz's timer.h.

Could we ditch timer.h and use std::chrono from C++11? In general, anything that we can get rid of in favor of now-standard C++ features is good (as long as it doesn't change the API; that is for later).

https://en.cppreference.com/w/cpp/chrono

As I said before... I don't care to maintain pre-C++11 compatibility going forward. We can go ahead and change the code along with the build system, as long as it doesn't change the API.

1
See my comment from this morning, I even believe that we can suppress the library (or at least most of its required uses for non debugging purposes) by using constexpr... Not sure whether suppressing the need for the library counts as an API change (people can always link with a non-necessary library)...

citibeth · 2019-02-22T14:24:08Z

Wow... that's quite a tour de force. But now I'm getting cold feet because it's 112 new files. I was hoping for something simpler. (I did a CMake build for Blitz in a proprietary setting for Windows, using far fewer files. Unfortunately, I cannot access or share that build. Then again, the build I made was a lot more minimal, just enough to get it working on Windows without re-generating code that someone else had already generated).

Overall, the mantra should be simplify, simplify. SOme things I can see right now I would want to remove:

Remove anything having to do with Pkgconfig. That is Autotools-related stuff, and it only works if everyone uses it --- which everyone never did.
Building the doc/ directory should be optional. Doc building requires a lot of extra build dependencies, and people should be able to build without first installing all that stuff. Most people will just use pre-built docs online. (This is true for almost any package that comes with docs).
Put all options in the top-level CMakeLists.txt file. That way it's easy to see what options this build has. (eg: FORTRAN_BENCHMARKS in in benchmarks/CMakeLists.txt)
Similarly, put all find_***() stuff in the top-level, so we can easily see what the dependencies are.
Remove commented-out Autotools code.
Remove bin/CMakeLists.txt, and the line that includes it.
The original Autotools build checked a zillion C++ features and configured accordingly. I would like to consider whether some of those checks are no longer required, for (reasonably) modern compilers. It is now 15+ years since Blitz++ originally was released, and plenty has changed. I don't care to support ancient compilers going forward; and people who DO use those compilers can always use the existing Autotools build. If we can remove checks, then that is super. (Later, we can remove the corresponding #ifdefs) For example... we can probably get rid of BZ_HAVE_STRINGS_H and BZ_HAVE_STRING_H. And a lot more...
Can we have a way to put the Blitz version number into the config.h file, coming from the top-level CMakeLists.txt file?
Is it possible to remove compiler-specific configuration (BlitzconfigFileName.cmake)? Configurations for specific compilers or OS's have fallen out of favor; instead, feature-based configuration options (like those discussed above) are preferred. In any case, can we remove support for compilers that no longer exist?
CheckCXXFeature: Again, let's get rid of checks for things that are now standardized. For example, HAVE_EXCEPTIONS, HAVE_NAMESPACES, HAVE_RTTI, HAVE_MEMBER_CONSTANTS, HAVE_OLD_FOR_SCOPING, HAVE_EXPLICIT, HAVE_BOOL,... Most of these (90%?) we can get rid of because they are now standard in C++. I would say, we should require C++11 (even though the current Blitz API is not C++11 compliant; I'm still not interested in having this CMake build work on any pre-C++11 compilers).
Let's try to put things in the file in which they are used. For example, CHECK_ALL_CXX_FEATURES is only used once, and that's in blitz/CMakeLists.txt. The macro should therefore be defined in that file, rather than in cmake/CheckCXXFeature.cmake. In general, any macro that's used in only one file should be inlined into that file.
cmake/Win32Compat.cmake: I'm happy not supporting CMake 2.6 any more. I'd be fine with requiring at least CMake 3.0. So we can get rid of extra code in the build required to support old CMake versions.
cmake/XXXConfig.cmake.in: Is there any way to avoid the situation in which the CMake build auto-generates part of itself? If we are to keep this kind of complexity, I would want a good reason.
Remove commented-out Autotools code from manual/CMakeLists/txt.

papadop · 2019-02-22T16:08:01Z

I just had a fast reading of your comments. I totally agree that it's more complicated than necessary nowadays. Please remember, that this was done years ago, at that time I wanted to provide the same functionality as with autotools. I just spent half an hour to separate this from other changes...

As for the goals of:

requiring C++-11
removing most of the testing macros (accidentally HAVE_STRING and HAVE_STRINGS might be the ones that we need to keep -- it is C no C++ and related to operating systems IIRC).
cleaning up the commented stuff in CMakeLists,txt (this was mostly reminders for eventual TODO work).

I totally agree with that (I would even go further and use #pragma once everywhere instead of include guards...). Then I'm not sure about the best strategy as doing all the work before merging might delay it for a while. I'll try to do some of the simple simplifications quickly, but my time is counted, and I'm unsure I can do all of your requests in the limited time I have. Can we find some middle ground ?

I'll make some initial cleanup and some of the suggested changes (the simplest ones).
We merge, so that the work remains synchronized with other changes and we can gather comments from users. Eventually, the testers can be extended to include a cmake build.
We do the remaining changes incrementally (and actually it would be nice to make them in both autotools and cmake world simultaneously).

I'll try to provide the initial cleanup over the week-end...

citibeth · 2019-02-22T16:24:30Z

I totally agree with that (I would even go further and use #pragma once everywhere instead of include guards...)

AFAIK, #pragma once is not standard. I would prefer we just stick with the C++11 standard, avoid using extensions.

. Then I'm not sure about the best strategy as doing all the work before merging might delay it for a while.

I'm not sure either. I can see arguments on both sides: a) Merge now because it works, and it doesn't conflict with / change any existing files b) Merge later because what we have now isn't what we eventually want. Now that I put it that way, I'd lean toward merging now. Even if the CMake build is totally broken (which I assume it is not), the merge STILL won't affect anything else. I think I'll be comfortable merging once the integration tests build and run successfully with CMake. ( @slayoo can you do this or tell us how to?)

I'll try do some of the simple simplifications quickly, but my time is counted, and I'm unsure I can do all of your requests in the limited time I have. Can we find some middle ground ? - I'll make some initial cleanup and some of the suggested changes (the simplest ones). - We merge, so that the work remains synchronized with other changes and we can gather comments from users. Eventually, the testers can be extended to include a cmake build. - We do the remaining changes incrementally (and actually it would be nice to make them in both autotools and cmake world simultaneously). I'll try to provide the initial cleanup over the week-end...

OK let's just stick with simple clean-up. Get rid of commented-out autotools code and the (one) vestigal file, make the doc/ directory optional or disable it, and (if there's time) move single-use macros into the file where they are used. Does that work?

….am). Made documentation building an option.

papadop · 2019-02-23T13:17:09Z

OK. I think I did the required changes...
By the way, I noticed various things.

The manual directory is mostly an old version of the doc directory. There are few differences in codes files though. I think this is confusing. The manual directory should be removed. At the very least, the all the Makefile.am(s) in this hierarchy should be removed.
There are bunch of .cvsignore remaining that should be suppressed.... That's an easy cleanup to do.

youngmit · 2019-02-23T20:45:01Z

@papadop Thanks for your work on this; I'm really excited for how much simpler Blitz++ can get once the CMake configuration is ready to replace autotools. I assume that is the plan?

Get a functional CMake configuration that can do everything that the autotools config can do (that we decide we wish to preserve, anyways)
Deprecate/remove the autotools build system
Remove all of the preprocessor macros that guard features that are standard as of C++11
Simplify the CMake config to not have to set all of those macros in the first place. e.g. remove the lion's share of the cxx_tests stuff.

edit: Sorry, used to rst

slayoo · 2019-03-03T20:48:50Z

will try this week to add Travis and Appveyor entries to this PR so the cmake build is tested during CI on Linux, OSX and Windows

citibeth · 2019-03-03T23:45:56Z

Awesome!

…

On Sun, Mar 3, 2019 at 3:48 PM Sylwester Arabas ***@***.***> wrote: will try this week to add Travis and Appveyor entries to this PR so the cmake build is tested during CI on Linux, OSX and Windows — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd4IbDWPwlLo4Q6T_xlEtWhcCoHWsks5vTDUzgaJpZM4bFK09> .

emmenlau · 2019-04-29T09:48:21Z

Hi, just a "thumbs up" for cmake support! Is this currently still being considered? For what its worth, I agree with @citibeth that merging sooner is better than merging later, and iterative improvements can be made when more people use/test the cmake support.

….am). Made documentation building an option.

…op-CMake

slayoo · 2019-04-29T21:21:53Z

apologies for the delay and thanks for the reminder.
Here's a first try to add CMake build to travis: papadop#2

trying to use also CMake on travis (+ merge with master)

Rebased on latest master and added option for doc

…build.

papadop · 2019-05-06T10:18:14Z

As for squash, I have no experience with it. I would have say "why" if there was many pure cmake related commits, but given that most commits are only for pure travis debugging, why not...

papadop · 2019-05-06T10:26:25Z

OK. That leaves the argument of taking time to get some feedback for the cmake build. Not that I want to insist on keeping the autotools build, just to make sure that people can fallback on the most used build in case something goes wrong. Plus I want to take time to verify that no autotools change creeped in after my cmake work (which was a while ago) and is not taken into account with the cmake build.

So let's keep the autotools build for now, and have the following strategy (if everyone agrees with it):

Advertise better the cmake build (to get more feedback).
After a while announce that the cmake way is the preferred one (autotools will be deprecated).
Remove the autotools (and also why not VS) files.

But, we need to agree on the plan and on a calendar !!!

slayoo · 2019-05-06T12:18:52Z

Also, for the record, I've added an item mentioning your CMake contribution here: https://github.com/blitzpp/blitz/wiki/Credits

citibeth · 2019-05-06T13:46:51Z

doc/CMakeLists.txt


-if (TEXI2PDF)
+if (TEXI2PDF AND PDFLATEX)


Not sure if this is what we need. To be clear, here is how these things SHOULD work: There should be a CMake option (looks like DOCS) that turns on generation of documentation. If that option is not set, then no docs are generated. If the option IS set, then docs will be generated. If the option is set and the required programs for generating docs do not exist (pdflatex and texi2pdf), then CMake should generate an error. It should NOT "automagically" decide whether or not to generate docs depending on whether it can find the required programs. That will cause people unending grief ("I set DOCS but docs weren't generated"), and is against best practices.

To be clear, I totally agree. This is a stop gap to allow compilation on travis MacOs which does not have latex and for which I do not know just like that how to install latex (+ the numerous latex packages which are needed). And I do not want to stop all documentation generation on MacOs just because of that (which would have been easier). Hence this hack.
I want to avoid adding too many documentation options, but I also want documentation building to be better tested.

I think we can revisit the problem later and just open an issue asking an enhancement for the time being.

citibeth · 2019-05-06T13:47:01Z

doc/CMakeLists.txt

@@ -66,8 +66,10 @@ endif()
 find_program(TEXI2PDF texi2pdf
             ${CYGWIN_INSTALL_PATH}/bin /bin /usr/bin /usr/local/bin /sbin)
 mark_as_advanced(TEXI2PDF)
+find_program(PDFLATEX pdflatex
+             ${CYGWIN_INSTALL_PATH}/bin /bin /usr/bin /usr/local/bin /sbin)


Yup. But it's not only pdflatex, it's fonts, packages, and problems with doxygen versions which generate bad latex... Believe me it's a can of worms I spent almost two days to dissect... and the public travis interface (without debugging) is a pain...

citibeth

Avoid "automagically" configuring a feature depending on whether required dependencies for that feature can be found.

papadop · 2019-05-06T14:14:31Z

Again, can we have an issue asking for such enhancements... As said above, the problem is much more complicated than only testing for pdflatex (in theory any other latex implementation could work too)...
Note that the autotool build does the same thing, and almost silently cancels some components when tools are missing.... or otherwise I would have not being obliged to add that many packages to the travis build. Look for the missing keyword at configuration time in the builds...

The cmake build already does much more testing than the autotools one. This is a net improvement. Let's already merge that and and create issues (not PR, sorry) for tracking required further enhancements.

citibeth · 2019-05-06T14:17:15Z

@papadop Looks like the easiest way to get docs built will be to figure out the correct dependencies (including versions); and then make a Spack package for it, that will build all the correct versions of Doxygen, etc. At that point I'm happy with a single documentation CMake option that either builds everything or nothing. As long as the Spack build knows how to build all the required dependencies, then we at least have a way to generate docs that doesn't require humans to go to a lot of installation effort just to build docs for one package.

citibeth

We will leave fixing doc generation for later.

citibeth · 2019-05-06T14:19:55Z

@slayoo If you can check this over and ensure it works with Travis, I'm happy to merge.

Before removing Autotools and Visual Studio, I want to make sure this works on Windows. CMake can generate VS builds. But the CMake files need to be tweaked differently to enable Windows functionality; at least in my experience.

slayoo · 2019-05-06T15:17:52Z

Re Doxygen, I'm a bit puzzled - IIUC this has nothing to do with the "good old blitz docs" that can be generated into the pdf.

It would be great to have the travis script upload somewhere the generated docs, and it's worth the effort, but of course there is no reason to wait for it in context of this PR - just created a separate issue: #122

slayoo · 2019-05-06T15:31:44Z

Re Windows: currently (i.e. with the branch behind this PR), CMake generates VS project files on Windows and compilation under VS is executed, no test execution happens, I've added an issue for it: #126

slayoo · 2019-05-06T15:41:09Z

@citibeth any opinion on merge vs. squash-and-merge?

@lutorm, @pguio any objections against merging it as is and continue development of further enhancements in separate branches?

I'm trying to go through all the comments above and create separate issues for the discussed features, I'll also try out the CMake build on my machine this evening and report back

citibeth · 2019-05-06T15:42:37Z

On Mon, May 6, 2019 at 11:41 AM Sylwester Arabas ***@***.***> wrote: @citibeth <https://github.com/citibeth> any opinion on merge vs. squash-and-merge?

Squash-and-merge is normally preferred.

slayoo · 2019-05-06T15:47:38Z

@Trophime, @tillea - if I'm not mistaken, you're listed as Blitz++ packagers in Debian. This PR includes initial version of CMake build scripts for Blitz++ intended as a replacement for autotools scripts. All thanks to @papadop!

We're in need of testing it and getting feedback from packagers - if possible, please test and report feedback. thanks!

slayoo · 2019-05-07T15:32:21Z

I've noticed that repeating "make check-testsuite" repeats compilation, while one would expect only the test execution to repeat (as there was no code change)? Is it intentional?

emmenlau · 2019-05-07T15:47:56Z

I've noticed that repeating "make check-testsuite" repeats compilation, while one would expect only the test execution to repeat (as there was no code change)? Is it intentional?

Does this hold true after a second, third and fourth time? I've seen that on complex projects, cmake can take one or two iterations until the cache does not change anymore.

slayoo · 2019-05-07T15:50:28Z

I've noticed that repeating "make check-testsuite" repeats compilation, while one would expect only the test execution to repeat (as there was no code change)? Is it intentional?

Does this hold true after a second, third and fourth time? I've seen that on complex projects, cmake can take one or two iterations until the cache does not change anymore.

yes, reproduced a few times with various intervals (but just on one machine)

slayoo · 2019-05-07T21:08:38Z

not sure if that matters, but in the generated install_manifest.txt file, the bzconfig.h entry is listed twice, which results in an error message when uninstalling:

$ sudo xargs rm < install_manifest.txt
rm: cannot remove '/usr/include/blitz/bzconfig.h': No such file or directory

citibeth · 2019-05-07T21:23:13Z

Sounds to me like a big.

…

On Tue, May 7, 2019 at 17:08 Sylwester Arabas ***@***.***> wrote: not sure if that matters, but in the generated install_manifest.txt file, the bzconfig.h entry is listed twice, which results in an error message when uninstalling: $ sudo xargs rm < install_manifest.txt rm: cannot remove '/usr/include/blitz/bzconfig.h': No such file or directory — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOVY57TDEV3IDWK4EWTYNTPUHVVNANCNFSM4GYUVU6Q> .

emmenlau · 2019-05-09T07:41:33Z

Thanks to @papadop and all!

papadop added 2 commits February 20, 2019 13:49

Correct test.

611c2cf

CMake build system.

ffc5219

papadop mentioned this pull request Feb 20, 2019

Create CMake Build #4

Closed

citibeth reviewed Feb 22, 2019

View reviewed changes

First cleanup (removed unused files and remainders of former Makefile…

f32c2f7

….am). Made documentation building an option.

papadop and others added 7 commits April 29, 2019 11:51

Correct test.

ec898b4

CMake build system.

8a6ae11

First cleanup (removed unused files and remainders of former Makefile…

7becb65

….am). Made documentation building an option.

CMakeLists.txt: Added option to cofigure documenation build

78b8b74

Improved and simplified blitz cmake configuration export

86b6f8a

Merge branch 'CMake' of https://github.com/papadop/blitz-1 into papad…

7ef19fa

…op-CMake

first try with travis and cmake

54e0033

papadop added 9 commits April 30, 2019 11:38

Merge pull request #2 from slayoo/papadop-CMake

1509f48

trying to use also CMake on travis (+ merge with master)

Merge branch 'CMake' into CMake

9b82b86

Merge pull request #1 from BioDataAnalysis/CMake

a178c05

Rebased on latest master and added option for doc

Attempt to correct travis script.

288e95b

Update version here too. Updated release date to date of 1.0.1.

aee9108

More attempt to correct travis.

2fdddf5

Dummy change just to trigger a travis build after some DNS problem.

3183ea9

Correct bug in travis script.

b6376b3

Add the options to build doc and tests for the travis/appveyor cmake …

3b88da9

…build.

citibeth reviewed May 6, 2019

View reviewed changes

citibeth requested changes May 6, 2019

View reviewed changes

citibeth approved these changes May 6, 2019

View reviewed changes

slayoo mentioned this pull request May 6, 2019

doc-generation option in CMake #127

Open

slayoo merged commit e8ec09f into blitzpp:master May 9, 2019

Cmake build #97

Cmake build #97

Conversation

papadop commented Feb 20, 2019

citibeth Feb 22, 2019

Choose a reason for hiding this comment

papadop Feb 22, 2019

Choose a reason for hiding this comment

citibeth Feb 22, 2019

Choose a reason for hiding this comment

papadop Feb 22, 2019

Choose a reason for hiding this comment

citibeth commented Feb 22, 2019

papadop commented Feb 22, 2019

citibeth commented Feb 22, 2019 via email

papadop commented Feb 23, 2019

youngmit commented Feb 23, 2019 • edited Loading

slayoo commented Mar 3, 2019

citibeth commented Mar 3, 2019 via email

emmenlau commented Apr 29, 2019

slayoo commented Apr 29, 2019

papadop commented May 6, 2019

papadop commented May 6, 2019

slayoo commented May 6, 2019

citibeth May 6, 2019 • edited Loading

Choose a reason for hiding this comment

papadop May 6, 2019

Choose a reason for hiding this comment

citibeth May 6, 2019

Choose a reason for hiding this comment

papadop May 6, 2019

Choose a reason for hiding this comment

citibeth left a comment

Choose a reason for hiding this comment

papadop commented May 6, 2019 • edited Loading

citibeth commented May 6, 2019

citibeth left a comment

Choose a reason for hiding this comment

citibeth commented May 6, 2019

slayoo commented May 6, 2019

slayoo commented May 6, 2019

slayoo commented May 6, 2019

citibeth commented May 6, 2019 via email

slayoo commented May 6, 2019

slayoo commented May 7, 2019

emmenlau commented May 7, 2019

slayoo commented May 7, 2019

slayoo commented May 7, 2019

citibeth commented May 7, 2019 via email

emmenlau commented May 9, 2019

youngmit commented Feb 23, 2019 •

edited

Loading

citibeth May 6, 2019 •

edited

Loading

papadop commented May 6, 2019 •

edited

Loading