Skip to content

ENH: Prevent cmake in source builds #1091

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

Merged

Conversation

hjmjohnson
Copy link
Contributor

Building directly inside the root of the source tree
can cause problems where the build intermediate files
overwrite or conflict with the intended source code
files.

This modification identifies this problem and
issues failure messages and suggestions to over
come the problem with more robust build suggestion.

Building directly inside the root of the source tree
can cause problems where the build intermediate files
overwrite or conflict with the intended source code
files.

This modification identifies this problem and
issues failure messages and suggestions to over
come the problem with more robust build suggestion.
@hjmjohnson hjmjohnson self-assigned this Nov 10, 2019
@hjmjohnson
Copy link
Contributor Author

See #1087 for motivation for this improvement.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.797% when pulling a3dccb5 on hjmjohnson:prevent-in-source-builds into 645cd04 on open-source-parsers:master.

@baylesj
Copy link
Contributor

baylesj commented Nov 13, 2019

It looks like this change requires built bot reconfiguration. If you can change the AppVeyor config to not build in the source directory, I'm happy to approve this patch.

@dota17
Copy link
Member

dota17 commented Mar 30, 2020

@hjmjohnson I think this issue can move forward now. Would you like to take a little time to look at this PR to move it forward?

@cdunn2001
Copy link
Contributor

@hjmjohnson, we've merged the clang-tidy fixes. Maybe that will help you fix VisualStudio? Otherwise, let's close this.

@baylesj
Copy link
Contributor

baylesj commented Nov 6, 2020

@cdunn2001, I think? I've fixed the appveyor issue, it's a valid one. I know our CMake support is deprecated but I'm still interested in landing this patch.

@baylesj
Copy link
Contributor

baylesj commented Nov 6, 2020

Travis is being slow, but it's all green so going to submit.

@baylesj baylesj merged commit 8954092 into open-source-parsers:master Nov 6, 2020
@gizmocuz
Copy link

gizmocuz commented Nov 9, 2020

Sorry for asking, but we use your library as a sub project and build this via:

add_subdirectory (extern/jsoncpp EXCLUDE_FROM_ALL)
target_link_libraries(domoticz jsoncpp_static)

With this change it's not possible to build this anymore

Could you make this an Option which is default set to YES, but when used as a external library in other projects we can set this to NO ?

@brucemiranda
Copy link

This has caused me a problem Building jsoncpp today. In the end I had to comment the two new lines added in the CMakelists within the json folder.

@gizmocuz
Copy link

I forked this repo, and now use my copy with also these two lines removed
This is not standard CMake, and it should not cause problems when building in the main folder and overwrite intermediate and source code files.
If it does, something else is wrong.
Sure it is best practice to create a build folder, but I would not force this...

@baylesj , @hjmjohnson , hope you can reconsider this patch

@cdunn2001
Copy link
Contributor

@baylesj , it was a good try, but I guess we need to revert at least some part of this. What do you think? The objections from these users seem valid.

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.

7 participants