Feature/generate export header #439
Conversation
|
I have commented on the other PR, but disable |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #439 +/- ##
===========================================
- Coverage 83.59% 83.59% -0.01%
===========================================
Files 24 24
Lines 8119 8117 -2
Branches 1694 1693 -1
===========================================
- Hits 6787 6785 -2
Misses 1332 1332
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
CMakeLists.txt
Outdated
| generate_export_header(Spglib_symspg | ||
| EXPORT_FILE_NAME spglib_export.h | ||
| EXPORT_MACRO_NAME API_SPGLIB | ||
| ) |
There was a problem hiding this comment.
Can you post a copy of the content of the generated spglib_export.h file?
I have a few design questions about this:
- Are the macros compiler independent, specifically on linux, would it be applicable for all compilers?
- On windows, how do
gccvsmsvccompilers work when they are cross-compiler consumed? - For Unix I would prefer to minimize the files populated in
/usr/include, and I'm not sure if it merits populating aspglibfolder for this. Probably it would work if the 2 files are concatenated before install, have you seen such design implemented?
There was a problem hiding this comment.
#ifndef API_SPGLIB_H
#define API_SPGLIB_H
#ifdef SPGLIB_SYMSPG_STATIC_DEFINE
# define API_SPGLIB
# define SPGLIB_SYMSPG_NO_EXPORT
#else
# ifndef API_SPGLIB
# ifdef Spglib_symspg_EXPORTS
/* We are building this library */
# define API_SPGLIB __attribute__((visibility("default")))
# else
/* We are using this library */
# define API_SPGLIB __attribute__((visibility("default")))
# endif
# endif
# ifndef SPGLIB_SYMSPG_NO_EXPORT
# define SPGLIB_SYMSPG_NO_EXPORT __attribute__((visibility("hidden")))
# endif
#endif
#ifndef SPGLIB_SYMSPG_DEPRECATED
# define SPGLIB_SYMSPG_DEPRECATED __attribute__ ((__deprecated__))
#endif
#ifndef SPGLIB_SYMSPG_DEPRECATED_EXPORT
# define SPGLIB_SYMSPG_DEPRECATED_EXPORT API_SPGLIB SPGLIB_SYMSPG_DEPRECATED
#endif
#ifndef SPGLIB_SYMSPG_DEPRECATED_NO_EXPORT
# define SPGLIB_SYMSPG_DEPRECATED_NO_EXPORT SPGLIB_SYMSPG_NO_EXPORT SPGLIB_SYMSPG_DEPRECATED
#endif
#if 0 /* DEFINE_NO_DEPRECATED */
# ifndef SPGLIB_SYMSPG_NO_DEPRECATED
# define SPGLIB_SYMSPG_NO_DEPRECATED
# endif
#endif
#endif /* API_SPGLIB_H */There was a problem hiding this comment.
This is the GCC and clang version for UNIX.
On Windows with DLL the export/import statement are different!
The 2 files seems ok for me in /usr/include, but a namespace subdirectory would break your API.
There was a problem hiding this comment.
The windows one is the one I want to see, as well as a gcc version on windows. I need to think if it makes sense to have it in a compiler specific version and what would happen with cross-compilation. There is conda and julia to consider.
There was a problem hiding this comment.
#ifndef API_SPGLIB_H
#define API_SPGLIB_H
#ifdef SPGLIB_SYMSPG_STATIC_DEFINE
# define API_SPGLIB
# define SPGLIB_SYMSPG_NO_EXPORT
#else
# ifndef API_SPGLIB
# ifdef Spglib_symspg_EXPORTS
/* We are building this library */
# define API_SPGLIB __declspec(dllexport)
# else
/* We are using this library */
# define API_SPGLIB __declspec(dllimport)
# endif
# endif
# ifndef SPGLIB_SYMSPG_NO_EXPORT
# define SPGLIB_SYMSPG_NO_EXPORT
# endif
#endif
#ifndef SPGLIB_SYMSPG_DEPRECATED
# define SPGLIB_SYMSPG_DEPRECATED __declspec(deprecated)
#endif
#ifndef SPGLIB_SYMSPG_DEPRECATED_EXPORT
# define SPGLIB_SYMSPG_DEPRECATED_EXPORT API_SPGLIB SPGLIB_SYMSPG_DEPRECATED
#endif
#ifndef SPGLIB_SYMSPG_DEPRECATED_NO_EXPORT
# define SPGLIB_SYMSPG_DEPRECATED_NO_EXPORT SPGLIB_SYMSPG_NO_EXPORT SPGLIB_SYMSPG_DEPRECATED
#endif
#if 0 /* DEFINE_NO_DEPRECATED */
# ifndef SPGLIB_SYMSPG_NO_DEPRECATED
# define SPGLIB_SYMSPG_NO_DEPRECATED
# endif
#endif
#endif /* API_SPGLIB_H */set CMAKE_C_VISIBILITY_PRESET too set and install PUBLIC FILE_SET HEADERS add the PRIVATE headers to library sources
done, but one test on windows still fails! |
| API_SPGLIB int niggli_reduce(double* lattice_, const double eps_, | ||
| const int aperiodic_axis); | ||
| API_SPGLIB void spg_set_error_code(SpglibError err); |
There was a problem hiding this comment.
NOTE: needed to prevent unresolved from tests found on OSX too!
src/CMakeLists.txt
Outdated
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT Spglib_Runtime | ||
| NAMELINK_COMPONENT Spglib_Development | ||
| ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT Spglib_Development | ||
| FILE_SET HEADERS INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} # COMPONENT Spglib_Development |
There was a problem hiding this comment.
This not clear to me, COMPONENT must not used here!
test/CMakeLists.txt
Outdated
| # package/pkg-config/src -> ../src/ | ||
| # package/FetchContent/src -> ../src/ | ||
| # package/find_package/src -> ../src/ | ||
| if (UNIX AND NOT PROJECT_IS_TOP_LEVEL) |
There was a problem hiding this comment.
(For some reason my comment got lost)
Please don't disable these. Can you explain the issue with git and symlink here? The other runs executed this fine
done, but one test on windows still fails!
Misleading because of diaabled tests. These tests are the main thing I've raised on discourse regarding export(EXPORT)
There was a problem hiding this comment.
Reply to: ClausKlein#1 (comment)
That does not work under windows, the symlinks and the installed export package is not found in that way you build this tests!
But on UNIX, it still works:
Symlinks on windows work fine, see develop tests failing for different reasons. Can you show me what errors git gives on a windows when it tries to create those?
On Windows, I dow the same in Visual Studio. only the install path is strange here?
Why is the install path strange?
There was a problem hiding this comment.
out/install/x64-debug/
Need more context, what's the tree around it.
There was a problem hiding this comment.
I open the directory in Visual Studio 2022, and a view second later, it configures the project and I can build, test, and install int. But I was not ask, which preset to use?
|
symlinks problems: |
That's a mac environment, and what problems do you encounter? The CI did not show any issues with using symlink, and I find links that that is not a problem for 4+ years 1 Footnotes |
On windows the |
Add .cmake-format and pip requirements.txt file too
9325e6b to
40b0d53
Compare
| @@ -0,0 +1,9 @@ | |||
| format: | |||
test/CMakeLists.txt
Outdated
| @@ -272,7 +272,10 @@ function(Spglib_add_test test) | |||
| endforeach () | |||
|
|
|||
| add_test(NAME ${ARGS_TEST_NAME} | |||
| COMMAND ${CMAKE_CTEST_COMMAND} --build-and-test ${CMAKE_CURRENT_SOURCE_DIR}/${test} | |||
| COMMAND ${CMAKE_COMMAND} -E env --modify PATH=path_list_prepend:${CMAKE_INSTALL_PREFIX}/lib | |||
There was a problem hiding this comment.
This should be in test property to make debugging more legible.
But for the usage in ctest --build-and-test I completely disagree with adding it. The example and package tests need to be able to test as standalone builds, like the user would have run it, and populating and ENV hides issues in thr CMakeLists.txt file that the user should know beforehand.
Note that testing-farm tests the case of standalone test project with system installed version. It does not cover Windows, but at the same time window does not have a packaging system we can upload to.
There was a problem hiding this comment.
This is the relevant part of the build log within visual studio 2020:
40/40 Testing: example-c_api
40/40 Test: example-c_api
Command: "C:/Program Files/Microsoft Visual Studio/2022/Professional/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/bin/cmake.exe" "-E" "env"
"--modify" "PATH=path_list_prepend:C:/Users/KLEIN_CL/Workspace/c/spglib/out/install/x64-debug/lib" "--"
"C:/Program Files/Microsoft Visual Studio/2022/Professional/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/bin/ctest.exe"
"--build-and-test" "C:/Users/KLEIN_CL/Workspace/c/spglib/test/example/c_api" "C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug/test/example/c_api"
"--build-generator" "Ninja" "--build-options"
"-D CMAKE_C_COMPILER=C:/Program Files/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.38.33130/bin/Hostx64/x64/cl.exe"
"-D CMAKE_CXX_COMPILER=C:/Program Files/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.38.33130/bin/Hostx64/x64/cl.exe"
"-D FETCHCONTENT_TRY_FIND_PACKAGE_MODE=ALWAYS"
"-D Spglib_ROOT=C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug"
"-D FETCHCONTENT_SOURCE_DIR_SPGLIB=C:/Users/KLEIN_CL/Workspace/c/spglib"
"-D SPGLIB_WITH_Fortran=OFF" "-DCMAKE_PREFIX_PATH="
"--test-command" "C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug/test/example/c_api/example" "&&"
"C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug/test/example/c_api/example_full"
Directory: C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug/test/example
"example-c_api" start time: Feb 19 09:41 Mitteleuropäische Zeit
Output:
----------------------------------------------------------
Internal cmake changing into directory: C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug/test/example/c_api
======== CMake output ======
The C compiler identification is MSVC 19.38.33135.0
Detecting C compiler ABI info
Detecting C compiler ABI info - done
Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.38.33130/bin/Hostx64/x64/cl.exe - skipped
Detecting C compile features
Detecting C compile features - done
Component omp of Spglib: NOT FOUND
Component fortran of Spglib: NOT FOUND
Found package: Spglib
Configuring done (5.3s)
Generating done (0.0s)
CMake Warning:
Manually-specified variables were not used by the project:
CMAKE_CXX_COMPILER
FETCHCONTENT_SOURCE_DIR_SPGLIB
FETCHCONTENT_TRY_FIND_PACKAGE_MODE
SPGLIB_WITH_Fortran
Build files have been written to: C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug/test/example/c_api
======== End CMake output ======
Change Dir: 'C:/Users/KLEIN_CL/Workspace/c/spglib/out/build/x64-debug/test/example/c_api'
Run Clean Command: C:/RsPython/3.12/Scripts/ninja.exe clean
[1/1] Cleaning all built files...
Cleaning... 0 files.
Run Build Command(s): C:/RsPython/3.12/Scripts/ninja.exe
[1/4] Building C object CMakeFiles\example.dir\example.c.obj
[2/4] Building C object CMakeFiles\example_full.dir\example_full.c.obj
FAILED: CMakeFiles/example_full.dir/example_full.c.obj
C:\PROGRA~1\MIB055~1\2022\PROFES~1\VC\Tools\MSVC\1438~1.331\bin\Hostx64\x64\cl.exe /nologo
-external:IC:\Users\KLEIN_CL\Workspace\c\spglib\out\build\x64-debug\src
-external:W0 /DWIN32 /D_WINDOWS /Ob0 /Od /RTC1 -std:c11 -MDd -Zi /showIncludes /FoCMakeFiles\example_full.dir\example_full.c.obj /FdCMakeFiles\example_full.dir\ /FS
-c C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(361): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(361): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(361): error C2133: 'rotation': unknown size
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(362): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(362): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(362): error C2133: 'translation': unknown size
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(392): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(392): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(392): error C2133: 'rotation': unknown size
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(393): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(393): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(393): error C2133: 'translation': unknown size
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(532): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(532): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(532): error C2133: 'grid_address': unknown size
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(533): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(533): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(533): error C2133: 'grid_mapping_table': unknown size
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(679): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(679): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(679): error C2133: 'pos': unknown size
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(680): error C2057: expected constant expression
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(680): error C2466: cannot allocate an array of constant size 0
C:\Users\KLEIN_CL\Workspace\c\spglib\test\example\c_api\example_full.c(680): error C2133: 'typ': unknown size
[3/4] Linking C executable example.exe
ninja: build stopped: subcommand failed.
<end of output>
Test time = 9.85 sec
----------------------------------------------------------
Test Failed.
"example-c_api" end time: Feb 19 09:41 Mitteleuropäische Zeit
"example-c_api" time elapsed: 00:00:09
----------------------------------------------------------
| @@ -7,4 +7,5 @@ add_test(NAME pytests | |||
| ) | |||
| set_property(TEST pytests APPEND PROPERTY | |||
| ENVIRONMENT PYTHONPATH=${Spglib_Python_BINARY_DIR} | |||
| PATH=${CMAKE_INSTALL_PREFIX}/lib:$ENV{PATH} | |||
There was a problem hiding this comment.
These tests should be run within the build directory without requiring install in order to support IDE debugging and rapid development
There was a problem hiding this comment.
Yes, I know, but step by step ....
test/example/c_api/CMakeLists.txt
Outdated
|
|
||
| if (UNIX) | ||
| add_executable(example_full example_full.c) | ||
| target_link_libraries(example_full PRIVATE Spglib::symspg) | ||
| endif () |
There was a problem hiding this comment.
Why does this not compile under windows?
There was a problem hiding this comment.
That issue I haven't managed to debug. This is avoidable by making it compile-time, (for some reason windows does not) or using malloc. One thing I haven't tested is if it's a C standard thing or if it's a flag/optimization thing
| @@ -357,7 +357,7 @@ static void example_spg_get_symmetry(void) { | |||
| {0.3, 0.3, 0.5}, {0.7, 0.7, 0.5}, {0.2, 0.8, 0.75}, {0.8, 0.2, 0.75}}; | |||
| int types[] = {1, 1, 2, 2, 2, 2, 1, 1, 2, 2, 2, 2}; | |||
| int num_atom = 12; | |||
| int max_size = 50; | |||
| const int max_size = 50; | |||
ToDo: First fix compile errors under windows!
5706f43 to
fcde3f5
Compare
Use CMAKE_RUNTIME_OUTPUT_DIRECTORY as CWD while test
3e4852b to
6933344
Compare
I give it up!This takes to mutch time and I recevie no support. |
|
Regarding generator expression I have mentioned that there are concerns I want cleared with upstream, particularly since there are 2 workflows that use cross-compilation. Regarding the dll, it is already covered in #437, including python, in a way that I believe is more reliable even though it's more verbose and ugly, and that you don't seem to agree with. The other compilation errors, I will get to it, but I just cover these bit-by-bit |
In general, generate_export_header() works fine!The only problem is to find the builded/installed DLL; cmake --workflow --preset default
pushd test
cmake --workflow --preset Release
popd
tree stagedir
stagedir/
|-- include
| |-- spglib.h
| `-- spglib_export.h
`-- lib
|-- cmake
| `-- Spglib
| |-- PackageCompsHelper.cmake
| |-- SpglibConfig.cmake
| |-- SpglibConfigVersion.cmake
| |-- SpglibTargets_shared-release.cmake
| `-- SpglibTargets_shared.cmake
|-- libsymspg.2.3.2.dylib
|-- libsymspg.2.dylib -> libsymspg.2.3.2.dylib
|-- libsymspg.dylib -> libsymspg.2.dylib
`-- pkgconfig
`-- spglib.pc
6 directories, 11 files
bash-5.2$ builddriver run-clang-tidy -checks='-*,clang-analyzer-*,cert-*' -p cmake-build-release src
builddriver executing: 'run-clang-tidy -checks=-*,clang-analyzer-*,cert-* -p cmake-build-release src'
Compilation SUCCEED in 6.276079 seconds
Number of warnings: 214
# ...Recommendation:
|
As I've said my concern is with cross-compilation, e.g. when it is built and packaged via msvc but then consumed on a gcc with wsl. The deprecated and import attributes would not be recognized. I am discussing with upstream whether this issue is relevant or if C has an ABI incompatibility that I should have known.
This is irrelevant, cmake presets are optional and users can use non-preset building for older versions. I will upgrade it to 3.25 when the CI for centos stream is resolved. The minimum is always tied to latest lts distro.
Cppreference does not indicate there is notable difference from 11 to 17. Do you feel like elaborating on this? C23 on the other hand is an exciting one to go for.
Which ones? The ones related with cmake version are there for an obvious reason because the minima is linked to diatro lts. For other ones, that's fair, I'll go over them after the 2 PRs I'm working one are done.
See the PRs. Right now we should focus on marking deprecation and documentation. Then I'll work on the C++ interface and slowly resolve these issues from top down. If you believe that's inappropriate, you are welcome to take over any of those PRs or submit auto-fixed ones. I have encountered regression errors when I tried mass fixes, but I was too unorganized and did not split it logically.
Those are intentional because those are unit tests. I couldn't find any reference on how to design around this issue.
Example-full I am aware of that, and I've mentioned 2 approaches that I want to try to fix. But finally, please do not assume incompetence on the upstream developers. I am not being paid to develop this code, and I don't think the other maintainers have funding or time to dedicate time to this. Everything is done from our personal time, and for the passion of the project. We are working on the issues you mention step-by-step, and although my approach is different, I always welcome any discussion on the PRs. I have given thought into those, but of course I may not have your experience, but please do try to take my viewpoint into consideration and discuss the design so we can both learn. |
Explicitly mark exported API
implemented according to cppcon-2019-deep-cmake-for-library-authors