Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CMake builds #36

Merged
merged 12 commits into from
Dec 2, 2021
Merged

Add support for CMake builds #36

merged 12 commits into from
Dec 2, 2021

Conversation

tlemo
Copy link
Contributor

@tlemo tlemo commented Nov 28, 2021

This is an experimental set of changes which add cross-platform build support using CMake (README.md has been updated to include CMake build instructions on Linux)

In addition to the CMake build files, there are a few bug fixes and C++ portability changes.

Hopefully, these changes may help a bit advance this nice project, at least as a starting point for cross-platform support.

Cheers!

@chrxh
Copy link
Owner

chrxh commented Nov 29, 2021

Nice, thanks a lot!

I'll will try out the CMake script on my system. Is your PR complete because you have set the status to "work in progress"?

Also thanks for pointing out and fixing the mentioned bug! I've now rewritten EngineWorker.cpp a bit so that it ensures that the image resource is registered to cuda after cuda is initialized. It could explain why some people had a crash at the first start (on the 2nd or 3rd run it apparently worked). See df41fa2.
(With this modification the additional resize call in _MainWindow::processRequestLoading is not necessary anymore.)

@Ccode-lang
Copy link

Can't wait to see this fully functional!

@tlemo
Copy link
Contributor Author

tlemo commented Nov 29, 2021

I'll will try out the CMake script on my system. Is your PR complete because you have set the status to "work in progress"?

With these changes you can build and run alien on Linux, so in that sense it's complete. The reason I marked the PR as draft is mainly to gauge the interest and get a chance to discuss some of the details, ex:

  1. How do you feel about using vckpg for third-party libraries, as opposed to embedding them under the external directory?
  2. The workaround for the _cudaSimulation initialization issue. I'm glad to hear that a real fix is already in place (so this part of the PR can be reverted)
  3. The simulation serialization format is currently not portable. In this PR, I removed the checked in autosave.sim and made a small change to start with the defaults when the file is not present. The portability goal is an open question.
  4. There are a number of warnings from GCC and Clang. Not a blocker but it may be worth to do a clean up pass (in a follow up PR)

Depending on how you feel about these details, the PR could be merged in.

@chrxh
Copy link
Owner

chrxh commented Nov 29, 2021

  1. How do you feel about using vckpg for third-party libraries, as opposed to embedding them under the external directory?

In principle, I like the idea of using external libraries in this way. Probably it's better to require a specific version in case there are compatibility issues?

I'd like to get your script working in Windows as well. But so far I have not been successful. After installing the external libraries via vcpkg and running cmake I get the error message
Could NOT find Boost (missing: Boost_INCLUDE_DIR serialization).
If I then set the directory manually using set(Boost_INCLUDE_DIR $ENV{VCPKG_ROOT}/installed/x86-windows/include), the next message comes up
Could NOT find GLEW (missing: GLEW_INCLUDE_DIRS GLEW_LIBRARIES) etc.
Do you know the reason why they are not found automatically?

Regardless of this, the CUDA part/engine was previously compiled to a separate dynamic library, as I originally had another executable target with automated tests in addition to the GUI (these are currently not contained here in this branch) where both used the engine. I'd like to add the automatic tests back soon and it would be more practical to build the engine part as a library.

  1. The simulation serialization format is currently not portable. In this PR, I removed the checked in autosave.sim and made
    a small change to start with the defaults when the file is not present. The portability goal is an open question.

That is a pity. Maybe it has to do with the size and byte order of integers?
In that case we have to convert the integers to bytes/chars manually... But this problem could be tackled separately from this PR.

  1. There are a number of warnings from GCC and Clang. Not a blocker but it may be worth to do a clean up pass (in a follow up PR)

Yes I agree, I also get some warnings with MSVC. A few have come in through include of glad.h.
Unfortunately, I haven't taken care of it so much yet.

@tlemo
Copy link
Contributor Author

tlemo commented Nov 29, 2021

  1. How do you feel about using vckpg for third-party libraries, as opposed to embedding them under the external directory?

In principle, I like the idea of using external libraries in this way. Probably it's better to require a specific version in case there are compatibility issues?

Adding explicit version requirements sounds like a good idea.

I'd like to get your script working in Windows as well. But so far I have not been successful. After installing the external libraries via vcpkg and running cmake I get the error message Could NOT find Boost (missing: Boost_INCLUDE_DIR serialization). If I then set the directory manually using set(Boost_INCLUDE_DIR $ENV{VCPKG_ROOT}/installed/x86-windows/include), the next message comes up Could NOT find GLEW (missing: GLEW_INCLUDE_DIRS GLEW_LIBRARIES) etc. Do you know the reason why they are not found automatically?

Have you installed the libraries with vcpkg install? (there are some notes in the README.txt, please let me know if there's any missing step)

I haven't got a chance to try in on Windows with MSVC yet, but I'm planning to do it shortly and I'll keep an eye for the issues you mentioned.

Regardless of this, the CUDA part/engine was previously compiled to a separate dynamic library, as I originally had another executable target with automated tests in addition to the GUI (these are currently not contained here in this branch) where both used the engine. I'd like to add the automatic tests back soon and it would be more practical to build the engine part as a library.

This sounds great - I was about to ask about this. The CMake scripts could be updated to factor out the components that way.

  1. There are a number of warnings from GCC and Clang. Not a blocker but it may be worth to do a clean up pass (in a follow up PR)
    Yes I agree, I also get some warnings with MSVC. A few have come in through include of glad.h. Unfortunately, I haven't taken care of it so much yet.

Understood. If I can carve the time, I may take a stab at cleaning some of the warnings (in a separate PR)

@chrxh
Copy link
Owner

chrxh commented Nov 29, 2021

Have you installed the libraries with vcpkg install? (there are some notes in the README.txt, please let me know if there's any missing step)

Yes. The includes for Boost and the other libraries are actually in $ENV{VCPKG_ROOT}/installed/x86-windows/include. I can get rid of this error message when I set the variable Boost_INCLUDE_DIR myself. When I do that, I keep getting further error messages for GLEW_LIBRARIES which can not fixed in that way (they are in .../lib). I will keep trying tomorrow.

I haven't got a chance to try in on Windows with MSVC yet, but I'm planning to do it shortly and I'll keep an eye for the issues you mentioned.

Thanks, that would be great.

@chrxh
Copy link
Owner

chrxh commented Nov 29, 2021

I managed to run the script. It was some x86 vs. x64 issue. I had to install the libraries via vcpkg install somelibrary:x64-windows.
But now I have a lot of linker errors due to the wrong insertion of the __declspec(dllexport)/__declspec(dllimport) decorators in Windows. This is because no DLL is created anymore in this build script.

EDIT: After removing the decorators, it worked! :-)

@tlemo
Copy link
Contributor Author

tlemo commented Nov 30, 2021

I managed to run the script. It was some x86 vs. x64 issue. I had to install the libraries via vcpkg install somelibrary:x64-windows.

Good catch. This is a bit of a trap on Windows (and one of the cons of using a package manager - you have an external build and have to deal with ABIs instead of building everything part of the project)

My solution is to set VCPKG_DEFAULT_TRIPLET env variable to x64-windows (assuming you're targeting x64). This can be done as a persistent env variable (System Properties / Env variables...) and you won't have to worry about it again (unless you want to build something targeting x86).

EDIT: After removing the decorators, it worked! :-)

Yay! Thanks for giving it a try!

@tlemo tlemo marked this pull request as ready for review November 30, 2021 01:44
@tlemo
Copy link
Contributor Author

tlemo commented Nov 30, 2021

I made a few refinements & tested the CMake build on Windows:

  1. Added a vcpkg.json manifest.
  • This specifies the dependencies, which will be automatically installed during cmake config phase (so manual vcpkg install no longer required)
  • The manifest file also defines the required versions
  1. Added vcpkg as a Git submodule under external/vcpkg
  • Further simplifies the setup steps (vcpkg doesn't have to be manually installed)
  • Offers isolation from other vcpkg locations
  1. CMakeLists.txt defines ALIEN_STATIC, which avoids the __declspec(dllimport/export) problems on Windows

NOTE: Because of the submodule usage, alien clones must use the --recursive flag (ex. git clone --recursive https://github.com/chrxh/alien.git), or submodule initialization after the clone step:

git submodule init
git submodule update

@chrxh
Copy link
Owner

chrxh commented Nov 30, 2021

That's great. It works for me right away! There are a few minor things I noticed:

  • The header files in ./source are not added to the target at the moment. Therefore, in my case, they are not visible in VS.
  • imgui.ini should be copied to the binary directory as a post-build step. The file contains all default positions and sizes of imgui windows.

Regarding the serialization problem:
Maybe we can use a text format instead of a binary format? According to https://www.boost.org/doc/libs/1_42_0/libs/serialization/doc/archives.html it would be portable. The size of the files increases by about 80% to 90%. I could live with that, if the portability is guaranteed.

For testing purposes I've serialized the startup example as text format. You can find it here: https://alien-project.org/files/autosave.zip
It would be kind if you can try it on your Linux system. In this case you need to change the following lines in Serializer.cpp:
boost::archive::binary_oarchive archive(stream); to boost::archive::text_oarchive archive(stream);
and
boost::archive::binary_iarchive ia(stream); to boost::archive::text_iarchive ia(stream);
and add the boost includes to it.
Does it work then?

@tlemo
Copy link
Contributor Author

tlemo commented Dec 1, 2021

  • The header files in ./source are not added to the target at the moment. Therefore, in my case, they are not visible in VS.

You're right. While header files are not technically required for builds, adding them should provide a better IDE experience - I'll add them.

  • imgui.ini should be copied to the binary directory as a post-build step. The file contains all default positions and sizes of imgui windows.

Will do.

Regarding the serialization problem: Maybe we can use a text format instead of a binary format? According to https://www.boost.org/doc/libs/1_42_0/libs/serialization/doc/archives.html it would be portable. The size of the files increases by about 80% to 90%. I could live with that, if the portability is guaranteed.

For testing purposes I've serialized the startup example as text format. You can find it here: https://alien-project.org/files/autosave.zip It would be kind if you can try it on your Linux system. In this case you need to change the following lines in Serializer.cpp: boost::archive::binary_oarchive archive(stream); to boost::archive::text_oarchive archive(stream); and boost::archive::binary_iarchive ia(stream); to boost::archive::text_iarchive ia(stream); and add the boost includes to it. Does it work then?

I'll give it a try and report back.

Thanks for the suggestions. I started making the changes, when I hit something that looks like a vcpkg bug, so I need to take a short detour to understand what's going on.

@chrxh
Copy link
Owner

chrxh commented Dec 1, 2021

Ok, thanks.

Thanks for the suggestions. I started making the changes, when I hit something that looks like a vcpkg bug, so I need to take a short detour to understand what's going on.

If I can help somehow please let me know.

- Individual CMakeLists.txt per subdirectory
- Add the header files (better IDE integration)
@tlemo
Copy link
Contributor Author

tlemo commented Dec 1, 2021

I added the headers and imgui.ini.

Since I see an interest in using this, I went ahead and did a proper refactor of the CMake scripts to follow the hierarchical project organization (one CMakeLists.txt per subdirectory).

Please take a look and let me know what you think.

PS. I tried the text-serialized autosave.sim. It solves the portability issue and it may be the easiest change (although there are portable serialization formats out there as well)

PPS. Here's the vcpkg issue I mentioned. Hopefully, the vcpkg would accept the fix, but in the meantime there are easy workarounds if anyone else hits it.

@chrxh
Copy link
Owner

chrxh commented Dec 1, 2021

When I build the project via

cmake .. -DCMAKE_BUILD_TYPE=Release
cmake --build . --config Release

then I encounter the error message:

alien_engine_gpu_kernels_lib.lib(CudaSimulation.obj) : error LNK2019: unresolved external symbol __cudaRegisterLinkedBinary_49_tmpxft_00007a74_00000000_8_CudaSimulation_cpp1_ii_27c0afcc referenced in function "void __cdecl __sti____cuda
RegisterAll(void)" (?__sti____cudaRegisterAll@@YAXXZ) [D:\temp\\alien\build\alien.vcxproj]
D:\temp\alien\build\Release\alien.exe : fatal error LNK1120: 1 unresolved externals [D:\temp\\alien\build\alien.vcxproj]

I could solved it by adding the following line in source/EngineGpuKernels/CMakeLists.txt:

set_property(TARGET alien_engine_gpu_kernels_lib PROPERTY CUDA_RESOLVE_DEVICE_SYMBOLS ON)

I don't know if this is the right way. I found it here (https://gitlab.kitware.com/cmake/cmake/-/issues/17520)
Then it worked also for me :) What do you think?

@chrxh
Copy link
Owner

chrxh commented Dec 1, 2021

After adding this line, everything seems fine to me and we can merge the PR. I will convert the simulation examples to the new format afterwards and also update the build instruction for Windows. Thanks again for your contribution! I'm happy that it now runs in Linux just like it does in Windows.

@tlemo
Copy link
Contributor Author

tlemo commented Dec 1, 2021

After adding this line, everything seems fine to me and we can merge the PR. I will convert the simulation examples to the new format afterwards and also update the build instruction for Windows. Thanks again for your contribution! I'm happy that it now runs in Linux just like it does in Windows.

The CUDA_RESOLVE_DEVICE_SYMBOLS workaround looks good to me, thanks! I incorporated the change into the PR.

@tlemo tlemo changed the title Experimental cross-platform build Add support for cross-platform build using CMake Dec 2, 2021
@tlemo tlemo changed the title Add support for cross-platform build using CMake Add support for cross-platform builds using CMake Dec 2, 2021
@tlemo tlemo changed the title Add support for cross-platform builds using CMake Add support for CMake builds Dec 2, 2021
@chrxh chrxh merged commit acaeee2 into chrxh:develop Dec 2, 2021
@chrxh
Copy link
Owner

chrxh commented Dec 2, 2021

Great, it's merged now.
I've updated the build instructions in the README.md a bit. Please check if it's still ok for Linux.

@tlemo
Copy link
Contributor Author

tlemo commented Dec 2, 2021

Great, it's merged now. I've updated the build instructions in the README.md a bit. Please check if it's still ok for Linux.

The build instructions look good to me. One thing that may be worth noting in there is that submodules may change and by default git pull doesn't update them. Here's a set of instructions I wrote for a different project on how to deal with this (I personally prefer to split the "getting the source" from the "build" steps instructions)

I noticed that you removed the MSVC project files too - it looks like a nice cleanup, it should be easier to maintain a single build system going forward.

PS. thanks for fixing the CMake comment!

@tlemo tlemo deleted the exp_cmake branch December 2, 2021 17:54
@chrxh
Copy link
Owner

chrxh commented Dec 2, 2021

Great, it's merged now. I've updated the build instructions in the README.md a bit. Please check if it's still ok for Linux.

The build instructions look good to me. One thing that may be worth noting in there is that submodules may change and by default git pull doesn't update them. Here's a set of instructions I wrote for a different project on how to deal with this (I personally prefer to split the "getting the source" from the "build" steps instructions)

Good point!
EDIT: darwin sounds like an interesting framework. I have to try this out at some time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants