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: set ENABLE_DEVELOPER_MODE to OFF by default + report the building mode #67

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

aminya
Copy link
Owner

@aminya aminya commented Feb 14, 2022

We should not expect the average user of a library pass flags to be able to use a library. Instead, the developer who knows the ins and outs of the library should set the developer mode to ON.

This not only affects the average end-users but also the advanced users that should go through the whole dependency tree and pass flags to each of them to make them usable as a dependency.

I should have paid attention to this in #46, but it is still not late to fix the default.

@aminya aminya added the bug Something isn't working label Feb 14, 2022
@aminya aminya force-pushed the dev-mode branch 2 times, most recently from 5b2154a to 23f9deb Compare February 14, 2022 20:37
We should not expect the average user of a library pass flags to be able to use a library. Instead, the developer who knows the ins-outs of the library should set the developer mode ON.
@aminya aminya changed the title fix: set ENABLE_DEVELOPER_MODE to OFF fix: set ENABLE_DEVELOPER_MODE to OFF by default + report the building mode Feb 15, 2022
Copy link
Contributor

@ddalcino ddalcino left a comment

Choose a reason for hiding this comment

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

Definitely worth doing.

src/DynamicProjectOptions.cmake Outdated Show resolved Hide resolved
Co-authored-by: David Dalcino <ddalcino@users.noreply.github.com>
@lefticus
Copy link
Contributor

My comment here has gotten away from me a bit, but I'm not sure of a better place for this comment right now:

I discussed with @aminya in a zoom chat a couple of weeks ago that I did intentionally make Developer mode the default. It is of very high importance to me, and related to the original reason I created this cpp_starter_project, that we nudge C++ developers in the direction of best practices. Specifically things like using sanitizers by default while developing.

  • It was a direct result of that and other surrounding discussions that I created the single DEVELOPER_MODE flag that make it one step to un-burden the user of the project from sanitizers and static-analysis
  • If DEVELOPER_MODE is not the default, it is unlikely to ever get enabled by the users of cpp_starter_project.
  • The options set here start to get us closer to being able to say "yes, there is a safe way to use C++ without hurting performance, you just have to Use The Tools"
  • This idea that there is a safe way to use C++ is directly core to my talks, books, and open source efforts

However, I also acknowledge:

  • inside of project_options might not necessarily be the best place to make it the default.
  • ENABLE_DEVELOPER_MODE=ON is enabled in the CI, so (if there are tests) the code is tested through ASAN, etc, there as well. While this is good, it does not help the C++ learner, it just teaches them to ignore the CI.
  • there are at least two different groups of C++ users we are trying to simultaneously reach
    • The experienced developer
    • The new developer

So, with these thoughts in mind

  • I'm going to approve this PR
  • I'm going to start to do a high-level reorganization of the projects with two goals
    1. return cpp_starter_project to its original goal of being a way for people who just want to get started with C++ and maybe play around in the sample projects (which means re-enabling as many of the sample binaries as possible)
    2. Create a new project (probably called cpp_boilerplate_project) that is almost completely bare
  • These projects will have a shared history so that merging of changes between them is easy when desired
  • I'm going to do a document reorganization so the goals of each project are clearly understood

When the reorganization is done:

  • Major new features will first be merged into cpp_boilerplate_project then merged back into cpp_starter_project which will diverge on things like defaults.

@ddalcino
Copy link
Contributor

I approved this under the assumption that developer mode would default to 'on' for cpp-starter-project, and 'off' if you just want to use project-options. I thought it was a really good compromise between the needs of the two projects. I guess I didn't realize that those defaults weren't set yet.

@lefticus
Copy link
Contributor

I approved this under the assumption that developer mode would default to 'on' for cpp-starter-project, and 'off' if you just want to use project-options. I thought it was a really good compromise between the needs of the two projects. I guess I didn't realize that those defaults weren't set yet.

I spent a lot of time starting and reading a Twitter conversation around these concerns and appreciate that there's room for a more nuanced response, but I also think it's time to have more than one fork of cpp_starter_project to address more users more easily.

@aminya
Copy link
Owner Author

aminya commented Feb 15, 2022

I agree with your idea. We should separate the beginner starter from the professional starter.

However, we should keep in mind that it is also important to communicate the outcomes of setting this as ON to beginners. I have seen people running Debug code for days on the government servers and causing thousands of dollars loss until they realized that they shipped the wrong binary mistakenly because they had set the defaults for the debug build.

@aminya aminya merged commit 56de45a into main Feb 15, 2022
@aminya aminya deleted the dev-mode branch February 15, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants