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

fix[next]: Default to Release in CMake #1420

Closed
wants to merge 1 commit into from

Conversation

havogt
Copy link
Contributor

@havogt havogt commented Jan 23, 2024

No description provided.

@havogt havogt requested a review from DropD January 23, 2024 16:52
@@ -44,7 +44,7 @@ class CMakeFactory(
"""Create a CMakeProject from a ``CompilableSource`` stage object with given CMake settings."""

cmake_generator_name: str = "Ninja"
cmake_build_type: BuildType = BuildType.DEBUG
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DropD please check if the default make sense in 3 different places and consider this example in the configuration project

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, sort of. I would argue that CMakeProject does not need to have a default value, since we never instanciate it directly. It makes sense though, that the factories can have separate default values.
Consider that using pure cmake leads to slightly simpler debugging, and in debugging the compilation times might not be as crucial.

There is, therefore an argument for leaving the default for CMakeFactory at DEBUG, while changing it to RELEASE for CompiledbFactory... However, in practice this might be more consfusing than helpful...

@havogt
Copy link
Contributor Author

havogt commented Feb 15, 2024

Fixed in #1438

@havogt havogt closed this Feb 15, 2024
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