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

Use CMAKE_CURRENT_BINARY_DIR instead of CMAKE_BINARY_DIR #76

Closed
wants to merge 1 commit into from

Conversation

Qix-
Copy link

@Qix- Qix- commented Feb 14, 2019

Otherwise, you pollute the project's build directory with a bunch of random stuff, which isn't very fun. 🙃

@codecov-io
Copy link

Codecov Report

Merging #76 into master will decrease coverage by <1%.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #76   +/-   ##
=====================================
- Coverage      88%    88%   -1%     
=====================================
  Files          41     41           
  Lines        3853   3854    +1     
=====================================
- Hits         3426   3422    -4     
- Misses        427    432    +5

@saprykin
Copy link
Owner

@Qix- ,
Thank you for the patch. Apparently, CMAKE_BINARY_DIR is also used for the tests, therefore it shall be adjusted there as well.

For me, the use of CMAKE_BINARY_DIR for output brings some advantages like a ready-to-deploy build folder, or that you can run compiled project directly from the build directory. I also understand that such a behaviour may be not beneficial for other projects. Therefore, I still want to keep a possibility to output compiled library into the top build directory.

I'm thinking about adding an option to control the output binary dir - either into the current binary directory, or into the top one. Any other suggestions how it could be done in a proper CMake-way?

@Qix-
Copy link
Author

Qix- commented Feb 24, 2019

@saprykin using CMAKE_BINARY_DIR is wrong pretty much in every scenario. I'd argue it should be deprecated from CMake entirely as it breaks hierarchical builds. Please do not use it.

@saprykin
Copy link
Owner

saprykin commented Jul 2, 2023

Okay, implemented in ac23d0a.

@saprykin saprykin closed this Jul 2, 2023
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.

3 participants