-
Notifications
You must be signed in to change notification settings - Fork 11
Fix cmake install #93
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
Conversation
|
Let me invite our CMake genius. @nickelpro btw @20162026 I might be inactive for about one to two weeks, I have an interview coming up so I won't have much time to focus on beman. Review might take longer :| |
nickelpro
left a comment
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 option() docstrings should get fixed, other comments are stylistic and not blocking.
Approved on the basis that everything is mechanically correct, but please fix docstrings prior to merge.
wusatosi
left a comment
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.
Feel free to merge once requested changes by @nickelpro is implemented, I just gave @20162026 write access to this repo.
|
I don't think there's any way to test if #92 is fixed locally... @neatudarius do you know anything about testing compiler explorer locally |
It should be testable and reproducible locally (at least compilers are), but in the worst case, I'll just check again after this PR is merged and CE updates deps. |
I am fine with this, setting up compiler explorer maybe a pain. |
nickelpro
left a comment
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.
LGTM ![]()
fix #80 and maybe #92 (yet to test CE locally)