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

cmake simplifications, static offsets and lightweight update generator #49

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

jll63
Copy link
Owner

@jll63 jll63 commented Jul 27, 2024

No description provided.

@jll63 jll63 requested a review from FabienPean July 27, 2024 00:23
@jll63
Copy link
Owner Author

jll63 commented Jul 27, 2024

@FabienPean Do you have time to take a look at the cmake bits? I removed the auto-download code. I feel that it does not make sense anymore, now that we have serious package managers.

Copy link
Collaborator

@FabienPean FabienPean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello there, just few comments around, but I cannot really dive into it at the moment

Comment on lines +52 to +53
if(CMAKE_COMPILER_IS_GNUCXX OR COMPILER_IS_CLANG)
add_compile_definitions(_GLIBCXX_DEBUG)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would add the definition on Windows if Clang is detected, not sure it matters though

Comment on lines +54 to +58
if (NOT (MSVC AND YOMM2_SHARED))
# Running this example with a Windows DLL is too much of a hassle, because we
# would need to add the path to the directory containing yomm2.dll to PATH.
# Anyway, if it works with static linking, it is very unlikely that it fails
# with the runtime in a DLL.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In recent CMake versions there is this generator expression that can help with DLL in case the example on Windows is desirable for completeness https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:TARGET_RUNTIME_DLLS

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting DLLs on Windows has cost me a huge amount of time and effort. I started work on v2, which will break backward compatibility. As a consequence of one of the changes (a small one for that matter) I plan to drop the shared library build, and have header-only the only option. Also, I doubt few people will ever use the generator, and, those who do, will most likely use a trimmed down policy. What I pre-instantiate in the DLL contains everything, even if the consumer is built for release.

"boost-mp11",
"boost-preprocessor",
"boost-test",
"vcpkg-tool-ninja"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never used vcpkg in manifest mode, so I am not sure, but is vcpkg-tool-ninja needed (or it might be a host only dependency? https://learn.microsoft.com/en-us/vcpkg/concepts/manifest-mode#package-manifest-example)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal is to make it as easy as possible for a developer, or CI, to grab everything that is needed to build the library, including test, examples and benchmarks. Yeah I considered moving the Ninja requirement to CMakeUserPresets.json but CI did not have it on Windows IIRC. Just being practical, it is a dev time only dep.

jll63 and others added 2 commits July 29, 2024 16:57
Co-authored-by: Fabien Péan <fabien@pean.pro>
Co-authored-by: Fabien Péan <fabien@pean.pro>
@jll63
Copy link
Owner Author

jll63 commented Jul 30, 2024

Thanks for the review!

@jll63 jll63 merged commit fcc3767 into master Jul 30, 2024
40 checks passed
@jll63 jll63 deleted the generator branch July 30, 2024 12:34
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.

2 participants