search

mbsim-env / fmatvec

A fast vector/matrix library
https://www.mbsim-env.de
GNU Lesser General Public License v2.1
10 stars 5 forks source link

Support Windows using MSVC #5

Closed GreenGary closed 2 years ago

GreenGary commented 4 years ago

Based on preparation made in isse #4 , the library shall be usable on Windows using MSVC toolchain. Target would be MSVC++ 14.1 _MSC_VER == 1910 (Visual Studio 2017 version 15.0; support for Visual Studio < 2015 would require more refactorintg, see this Microsoft document.

Todos:

Remarks:

=> issue to be discussed!

GreenGary commented 4 years ago

@rolandzander FYI

friedrichatgc commented 4 years ago

At least in combination of fmatvec with MBSim a so/dll of fmatvec is required: fmatvec uses boost::spirit for parsing vectors/matrices as well as symbolic expressions. Since vector/matrix/expressions are all templated we explicitly instantiate MANY common template variants in the so/dll. If not doing so all the boost::spirit code must be compiled in every program using fmatvec (MBSim). This increases the compile time of MBSim by factors.

Handling of the lib file should work out of the box in cmake.

All the declspec things for windows may be problematic for a dll, but I think its just work to do.

GreenGary commented 4 years ago

@friedrichatgc I have found another compilation issue, I am not sure how to address. The friend declarations in class Operation are not considered by the MSVC compiler. Error is fmatvec\ast.cc(645): cannot access private member declared in class 'fmatvec::AST::Operation'

Fixed: Missing namespace confused MSVC - will drop a pull request to fix this.

GreenGary commented 4 years ago

@friedrichatgc I would like to know your opinion about the dllexport/dllimport issues, when using MSVC. I played around with cmake / msvc but cannot make a final decision on that.

gcc linux build exports about 2.000 symbols, mingw about 1.500 in total (global, weak, undefined, text, code, ...)

  1. The most general approach to export symbols is using set_target_properties( fmatvec PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS 1 ) for the dll. This exports ~17.000 symbols and increases dll/lib's size considerable. In addition, it does not fix dllexport/dllimport issues with static variables defined in the dll (e.g. static unsigned long evalOperationsCount;)
  2. Prefix all exported symbols with a macro and set macro to __declspec(dllexport) or __declspec(dllexport) (or empty for gcc).
  3. Maintain exports in a separate def file
  4. Call cmake -E __create_def only on some of the obj files to generate the def
  5. Create the def file on all/some obj files and filter the output.

My thoughts about potential solutions:

  1. Will cause issues in real life due to duplicate symbols etc. ; increase in size is not acceptable.
  2. Would be the most declarative way, but it infringes the complete code base - and it is just clutter for gcc build.
  3. Maintaining the def file manually will break MSVC build each and every time - especially with all the templated stuff, it will become a nightmare to maintain; same issue with static variables as in 1.
  4. Might be a good approach, if a subset can be defined. I do not know which obj would be the right subset - I tried linear_algebra_complex.cc.obj linear_algebra_double.cc.obj which returned a reasonable list of symbols. Same issue with static variables as in 1.
  5. May always be a little fuzzy, but maybe a good compromise (if 2. is not an option). Same issue with static variables as in 1.
GreenGary commented 4 years ago

I created a prototype in my fmatvec-fork with a combination of 4. and 5. : https://github.com/GreenGary/fmatvec/tree/2020-04-20-cmake-msvc

friedrichatgc commented 4 years ago

I think the best and most common way is option 2:

This is also the "todays" usual way for gcc and does not clutter gcc. But sure it requires a lot of changes in the code. I don't like any option which decouples the code (.h file) and the export definition (.def file). Both should be in the same file -> use declspec/attribute__.

GreenGary commented 4 years ago

@friedrichatgc I am currently setting up the required dll exports using __declspec. I noticed, that most matrixes have prepared shift operators, but not DiagMat . Any matrix except DiagMat can be linked without boost::spirit::qi::rule being exported. Is this missing from stream.h and stream.cc like below? And why does this not compile?

extern template std::istream MSVC_DLL_DECLSPEC & operator>>(std::istream &s,       Matrix<Diagonal, Ref,     Ref,     double              > &A);
extern template std::ostream MSVC_DLL_DECLSPEC & operator<<(std::ostream &s, const Matrix<Diagonal, Ref,     Ref,     double              > &A);

Found while trying to build testfunction.cc: { istringstream str(inputd); DiagMat m; str>>m; cout<<m<<endl; }

friedrichatgc commented 4 years ago

Building testfunction.cc should work without DiagMat explicitly noted in stream.h|.cc since testfunction.cc include stream_impl.h. So I don't know what is wrong here but we can also add DiagMat to stream.h|.cc. I have just pushed this (maybe you forgot to include diagonal_matrix.h). If this help fine but I have no idea why boost::spirit::qt::rule needs to exported? It from a boost header only lib.

GreenGary commented 4 years ago

I have a draft for the library exports using declspec / attribute ((visibility ("default"))) : https://github.com/GreenGary/fmatvec/tree/control-exported-symbols fmatvec and its checks build successfully. Library now exports 660 symbols less than on the master branch (2214). I tried to export only reasonable stuff, maybe some symbols are missing now. However, I do not get linker errors when running mbsim build script. However, the script raises an error (which as far as I understand, is no related to linkage):

/mbsim-env/mbsim/kernel/mbsim/element.h:56: Error: 'fmatvec::Atom' is not a valid base class.
/mbsim-env/local-docker/include/fmatvec/atom.h:115: Error: See definition of 'fmatvec::Atom'.
/mbsim-env/mbsim/kernel/mbsim/element.h:56: Warning 401: Nothing known about base class 'fmatvec::Atom'. Ignored.

Since I do not know anything about SWIG, I can just guess, this error comes from the additional macro?

friedrichatgc commented 4 years ago

I have added this branch to mbsim-env/fmatvec and enabled this branch for CI, see https://www.mbsim-env.de/mbsim/linux64-ci/report/result_0000000409/index.html I will fix the SWIG problem next.

friedrichatgc commented 4 years ago

mbsim/master is now updated to work with fmatvec/control-exported-symbols, see https://www.mbsim-env.de/mbsim/linux64-ci/report/result_0000000413/index.html

friedrichatgc commented 4 years ago

The staging build system at wwwstaging.mbsim-env.de (only accessible via IPv6) is now ready to build mbsim tools using cmake/ninja. The staging system is configured to use the fmatvec cmake branch as default instead of the master branch. If a CMakeLists.txt file is found in the basedir then cmake/ninja is used, if not autotools are used for building. The requirements of cmake/ninja based tools are that the following targets exists:

The first three are default cmake targets. The check target is also already implemented on the cmake branch. The doc/ targets are missing. This is also the reason for all failures of the doc build of all other tools. xmldoc/ is not needed for fmatvec.

All failures seen on the staging build system seem to be related to missing parts of the cmake/ninja build of fmatvec. But now we have a working testing environment for the complete project using the cmake/ninja fmatvec variant.

rolandzander commented 4 years ago

I will try to address the targets "doc/..." within the next two weeks. Are there other open issues to be resolved before the cmake build can fully be replacement when building fmatvec?

In case of fmatvec, I see no conflict in merging branch "cmake" to "master". Shall we plan to extract the automake parts to a separate branch and make "master" provide cmake including native Win build? Shall both cmake and automake be continuously maintained.

Am 16.07.20 um 19:25 schrieb Markus Friedrich:

The staging build system at wwwstaging.mbsim-env.de (only accessible via IPv6) is now ready to build mbsim tools using cmake/ninja. The staging system is configured to use the fmatvec cmake branch as default instead of the master branch. If a CMakeLists.txt file is found in the basedir then cmake/ninja is used, if not autotools are used for building. The requirements of cmake/ninja based tools are that the following targets exists:

  • the default target = all = "none specified": build the actual tool
  • clean: remove all files generated by all
  • install: install the tool to CMAKE_INSTALL_PREFIX
  • check: build and run "unit" tests of the tool
  • doc/all (only if a subdir named doc exists): build doxygen documentation
  • doc/clean (only if a subdir named doc exists): clean the files generated by doc/all
  • doc/install (only if a subdir named doc exists): install the doxygen documentation
  • xmldoc/all (only if a subdir named xmldoc exists): build xml documentation
  • xmldoc/clean (only if a subdir named xmldoc exists): clean the files generated by xmldoc/all
  • xmldoc/install (only if a subdir named xmldoc exists): install the xml documentation

The first three are default cmake targets. The check target is also already implemented on the cmake branch. The doc/ targets are missing. This is also the reason for all failures of the doc build of all other tools. xmldoc/ is not needed for fmatvec.

All failures seen on the staging build system seem to be related to missing parts of the cmake/ninja build of fmatvec. But now we have a working testing environment for the complete project using the cmake/ninja fmatvec variant.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mbsim-env/fmatvec/issues/5#issuecomment-659556874, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADHK2DG4HANVZSMGPHZQR33R34ZYHANCNFSM4LWSC5KA.

friedrichatgc commented 4 years ago

Just all issues on wwwstaging.mbsim-env.de must be resolved before merging. The "make" error in "hdf5serie/h5plotserie" seems also to be related to fmatvec: fmatvec seem not to put all its dependencies to fmatvec.pc (at least the boost include dir seem to be missing). The Windows build of fmatvec is also not working: seems to be a missing blas lib when linking (on Linux linking against lapack will implicitly link with blas but on Windows such implicit linking does not take place, as far as I know). More errors may be seen if the current ones are fixed.

friedrichatgc commented 4 years ago

I would remove the autotools part from the cmake branch and if all works on wwwstaging merge it to master. Before merging fmatvec the staging branch of the build repo must be merged to master to enable cmake on www.mbsim-env.de. Supporting both cmake and autotools is no option for me.

GreenGary commented 4 years ago

The Windows build of fmatvec is also not working: seems to be a missing blas lib when linking (on Linux linking against lapack will implicitly link with blas but on Windows such implicit linking does not take place, as far as I know).

I successfully used LAPACK/BLAS with x86 builds from http://icl.cs.utk.edu/lapack-for-windows/lapack/LAPACKE_examples.zip - these were the only MSVC builds (not mingw) I found. Without an Intel Fortran on hands, I could not build myself. Roland links against IntelMKL. So my question is: Which BLAS/LAPACK is in use on the CI system?

friedrichatgc commented 4 years ago

The CI system builds lapack/blas from source using mingw-cross-compiler, see https://github.com/mbsim-env/build/blob/master/docker/buildwin64Image/Dockerfile Since this build is not found by fmatvec-cmake automatically the system provides to cmake the variables BLAS_LIBRARIES, LAPACK_LIBRARIES, BLAS and LAPACK, see https://github.com/mbsim-env/build/blob/staging/docker/buildwin64Image/entrypoint.py But cmake seems just to past the lapack liberary to the linker command but not blas, see https://wwwstaging.mbsim-env.de/base/textFieldFromDB/builds/Tool/1309/makeOutput/

rolandzander commented 4 years ago

I try to fix the windows build testing and accouting for BLAS.

The boost-include path thing to me seems to be a problem of fmatvec AND h5plotserie: all other packages depending on fmatvec succeed in their build, and h5plotserie can not resolve include of boost/filesystem.h which is not required by fmatvec. Unfortunately I do also not find include directives in the fmatvec.pc of the autmake build+install, so no valid source for me to copy from! I now add include directive to cmake-generated fmatvec.pc since fmatvec definetly carries that dependency - still h5plotseries seems unclear to me.

friedrichatgc commented 4 years ago

Note that wwwstaging.mbsim-env.de is currently not running since everything is merged now to www.mbsim-env.de which does not CI also for the fmatvec cmake branch and also does the nightly builds (including windows) for the fmatvec cmake branch. See "Continuous Integration (CI) Linux64 Debug Build" and "Daily builds of none default branches" on www.mbsim-env.de

You will see tomorrow the results of your changes their (these builds run between about 6:00 and 8:30).

The boost include thing I see as a pure fmatvec problem since h5plotserie just correctly relies on the implicit dependency to boost which is brought in by fmatvec (all other tools just have a explicit dependency to boost either due to additional binary boost library requirements or due to historical reasons which fmatvec does not require boost at all). Adding the proper boost include directive to fmatvec.pc should resolve the issue as it is done by the autotools generated fmatvec.pc (the @DEFINE@ variable pushes the boost include their).

rolandzander commented 4 years ago

For the "doc" and "xmldoc" parts we might need different target names (for all/clean/insall), since cmake does not support the definition of target names with (forward) slashes, see https://gitlab.kitware.com/cmake/cmake/-/issues/20774. Kitware suggests to implement targets like "doc-all/doc-install/doc-clean" instead. Does this collide with the current build definition? Automake allows for target grouping via directories, I don't see a comparable mechanism for cmake?!?

friedrichatgc commented 4 years ago

I added it to the build system as target doc/all due to this issue https://gitlab.kitware.com/cmake/cmake/-/issues/16677 This seems to be in contrast to the issue you found. But I can change the target name the build system uses for cmake/ninja to doc-all, doc-install, ... or maybe better doc, doc-install, ...?

rolandzander commented 4 years ago

The drawback of adding results of target doc to install via cmake's install(DIRECTORY html ...) is that doc will be added to top level install target, which is in contrast to current behavior and would completely mix up current separation. Also I did not see a simple way to introduce the dependency of install on target doc in that case. cmake's mechanism for separation of installation parts is the COMPONENT specification in command install(...) as used in current commit. If changing target names, and these will be common for GNU-make and ninja when not using slashes, I do not insist on any naming convention - feel free to suggest or change as you like (as you can see, doc-all is only alias for doc which I preferred when implementing).

friedrichatgc commented 4 years ago

I don't understand your comment but will change the targets to doc, doc-clean, doc-install.