search

dev-cafe / autocmake

CMake plugin composer.
http://autocmake.org
BSD 3-Clause "New" or "Revised" License
42 stars 18 forks source link

Adaptation for static linking #54

Closed miroi closed 9 years ago

miroi commented 9 years ago

Hereby I am asking for the code review first (while doing the git rebase in between).

bast commented 9 years ago

Hi Miro. Generally I think that you submit the PRs way too early. But I understand why. The earlier the better for you since you get feedback earlier before all that coding. The later the better for me since the product is more converged. Difficult balance but in general please let the code first ripe on your fork for few days/weeks to debug the corner cases. You don't need the code to immediately enter the central repo thanks to distributed version control. You can develop multiple features at the same time without the features entering immediately the central repo. What is the central repo anyway in Git?

Comments:

miroi commented 9 years ago

Hi Rado,

when my code is working on branch, and I foresee no substantial changes in the future, why waiting ? Both fresh and ripe codes will undergo review anyhow, and I don't believe that an 'old' code will have less referee comments than a 'fresh' one, as is this one.

Answer to comments:

i) I renamed LINKING_STUFF to MATH_LIBS_EXT in corresponding CMakeFiles.txt files. I am against putting the LINKING_STUFF -> MATH_LIBS_EXT variable into math_libs.cmake as for static linking it differs from compiler to compiler, from library to library (and even from version to version). Let the MATH_LIBS variable provides the very core for static linking, and remaining adaptations for various cases of static linking are to be done on user's side. Such is my experience with combination of Intel, PGI, GNU compilers / MKL,ATLAS, SYSTEM_NATIVE math libraries.

Even I was shaking with "holy awe" before modifying math_libs.cmake for static linking. Yes, I modified this file, in the minimalist and IMHO sufficient way.

ii) Packages added to .travis-ci.yml are aimed to ensure proper static linking on CI testing servers. Especially the fc_lapack static linking is non-trivial. After experimenting (how many commits were shot for that !) I had to resort to the static ATLAS library, and had switch linking order to "lapack-blas" in math_libs.cmake. One important case : only the binutils-gold package is able to ensure proper static linking on Linux (fortunately it exists on travis-ci Linux server(s), on my clusters I had to install it manually).

With all these proposed changes for autocmake I am heading to the new static autocmake-DIRAC software to be used for multinode grid computing. I am sure the "autocmaked" DIRAC is better starting point in comparison to what I had before.

bast commented 9 years ago

Hi Miro, just one thing about the waiting: You typically implement something that you need and you use. It is normal and typical that you discover bugs while using the new feature. It would be extremely atypical to code something that is just ready and bugfree and does not require refactoring or small or large changes. Therefore it is good to wait with the PR and use the code that you have written for some time before submitting it for a PR.

miroi commented 9 years ago

Fine, I am going to perform all-combinations tests on this new feature and will display results in tabular form.

bast commented 9 years ago

Hi Miro, you don't have to do it for me. I don't need any "executive summary". I just recommend that you use the feature that you have implemented for few days before filing a PR (unless it is a trivial change).

bast commented 9 years ago

The test CMake sources should IMO be "thin". If all the static lib adaptations are in the test sources but not in CMake modules that we tests, then I am not sure what we test in them. So either it can be done in a portable way and then it needs to be in a module or it cannot be done and then I think we should not even attempt it.

miroi commented 9 years ago

Currently what is working is static linking combination of gfortran+ATLAS/SYSTEM_NATIVE/MKL, ifort+ATLAS/SYSTEM_NATIVE/MKL, pgf90+ATLAS/SYSTEM_NATIVE.

The wanted (as prerequisite for static-DIRAC-for-grid) combination of _pgf90+MKL+static _ is not working due to this error:

ilias@login.grid.umb.sk:~/Work/qch/software/software_projects/autocmake_devel/autocmake_miroi/test/fc_blas/build_static_pgf90_mkl/.m
Linking Fortran executable ../bin/example
IPA: no IPA optimizations for 1 source files
/mnt/apps/intel/composer_xe_2013_sp1.1.106/mkl/lib/intel64/libmkl_core.a(mkl_aa_fw_load_orsl_lite_lib.o): In function `mkl_aa_fw_load_orsl_lite_lib':
../../../../serv/offload/framework/core/mkl_aa_fw_load_orsl_lite_lib.c:(.text+0xbe): warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/usr/lib64/libpthread.a(libpthread.o): In function `sem_open':
(.text+0x774d): warning: the use of `mktemp' is dangerous, better use `mkstemp'
/usr/bin/ld: dynamic STT_GNU_IFUNC symbol `strcmp' with pointer equality in `/usr/lib64/libc.a(strcmp.o)' can not be used when making an executable; recompile with -fPIE and relink with -pie
child process exit status 1: /usr/bin/ld
make[2]: *** [bin/example] Error 2
make[1]: *** [src/CMakeFiles/example.dir/all] Error 2
make: *** [all] Error 2
milias@login.grid.umb.sk:~/Work/qch/software/software_projects/autocmake_devel/autocmake_miroi/test/fc_blas/build_static_pgf90_mkl/.

I made quite heavy adaptation of the test's src/CMakeFiles.txt files . Maybe there is a space for moving some local cmake commands into the common _staticlinking.cmake module. But first I would like to fix pgf90+mkl+static , later trying to make static tests thin.

Comments, please ?

bast commented 9 years ago

Whatever works is fine. If this helps you achieving your goal then it is a good route. Later you can refactor.

miroi commented 9 years ago

Hi,

finally I got PGI+MKL+static working !. I strived to keep minimum changes into modules/math_libs.cmake, so that the complexity of various compilers/libraries/linking options remains at user's src/CMakeFiles.txt side. There is no other way, IMHO.

All 9 combination of static linking (pgf90,ifort,gfortran/ATLAS,SYSTEM_NATIVE,MKL) is working (my "execution summary" on Linux cluster)!

I am missing the ACML library (3 more possibilities to check, maybe 6 with the ilp64 mode, or in total 12 when testing both dynamic and static linking) and the OpenBLAS library (another bunch of combinations for testing...). Also one should test it with IBM XL/ESSL, SYSTEM_NATIVE .... damn, another testing options...

So I am asking for this PR as I have serious plans with the static autocmake-DIRAC. The Slovak (empty) Grid environment is waiting.

bast commented 9 years ago

Sorry my changes force you to rebase and resolve but the conflicts should be trivial.

miroi commented 9 years ago

Rebased.Any other changes?

bast commented 9 years ago

Hi Miro, this is very important work. Thank you for that. The only thing that I don't like about this change is that the tests carry all the workarounds and not the modules. This means that the tests pass but if a user wants to use the module for static-linking it will then not work and one would need to reintroduce the workarounds every time. Isn't there a way to hide those away in a module? The test sources should ideally be free of hacks and workarounds otherwise there is IMO no point for these tests. The tests now test themselves and not the modules.

miroi commented 9 years ago

Fine,then Iet us wait,more iterations of this PR are to be done.I hope a lot of "hacks" will go to the mathlibs.cmake.

miroi commented 9 years ago

Comments please ?

bast commented 9 years ago

I think this is good. We want the test sources to be thin. The reason is that we want users to be able to plug CMake functionality with writing minimum CMake code on their side.

miroi commented 9 years ago

Really good ? And concerning https://github.com/scisoft/autocmake/pull/54#discussion_r36615453 ?

miroi commented 9 years ago

Green tests; merge ?

bast commented 9 years ago

Thanks Miro! I am not 100% happy that we so heavily modify link flags that come from CMake but perhaps we have to live with it for a while. Perhaps there is no other way. I hope there is but we can explore it with the code in. Why is the [explicit] block needed in fc_blas test?

miroi commented 9 years ago

fixed and rebased

bast commented 9 years ago

Hi Miro, can you please try one more thing:? Currently we "brutally" erase all link flags with

set(CMAKE_SHARED_LIBRARY_LINK_Fortran_FLAGS)

Can you please try whether this works also:

list(REMOVE_ITEM CMAKE_SHARED_LIBRARY_LINK_Fortran_FLAGS "offending-flag")

This would be more gentle and more explicit.

miroi commented 9 years ago

Hi, works. In general, for cases as this one I would prefer an automatic CDash testing. PGI/Intel/GNU/mathlibs/int64/static libs give too many combinations alltogether. We can not rely only on CI tests with free compilers and libs. Other projects, candidates for utilizing Autocmake, would prefer the best compilers/libraries at hand, and these are usually commercial.

bast commented 9 years ago

Thanks Miro. I have nothing against automated CDash testing in addition to the Travis/Appveyor CI. But my ambition is to work towards a portable compiler-independent Autocmake which can be tested by Travis/Appveyor.