-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Follow up for the CMake refactoring #1249
Conversation
f900e8b
to
46e7465
Compare
46e7465
to
d2f3a07
Compare
The new CMake summary prints this:
Why does |
@mavam that variable is defined by CMake itself, we just consider it important enough to include it in the summary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch builds fine for me, and since CI is green, I think green-lighting this makes sense.
I do have a few questions though.
Can we remove this "noise" in the CMake run?
The all-caps comments probably don't need to be in here, right? |
Maybe move it to the top/bottom then? Otherwise it looks like a bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After your latest commit, my only remaining concern would be the left-over TODO.
It was always enabled by default and there is currently no reason to support this.
It is now possible to build VAST with a fixed schema path that is different from `CMAKE_INSTALL_DATADIR`.
654a102
to
5c58198
Compare
📔 Description
VAST_ENABLE_ASSERTIONS
was broken.-Wundef
config.hpp
📝 Checklist
🎯 Review Instructions
By commit.