Skip to content

add cmake variable RAPIDJSON_INSTALL to control installations#1957

Open
ouonline wants to merge 1 commit intoTencent:masterfrom
ouonline:dev
Open

add cmake variable RAPIDJSON_INSTALL to control installations#1957
ouonline wants to merge 1 commit intoTencent:masterfrom
ouonline:dev

Conversation

@ouonline
Copy link

Users can disable installations by setting RAPIDJSON_INSTALL to OFF

@dinomight
Copy link

What's the purpose of disabling installations? My understanding of CMake says these don't actually do anything unless you run CMake install. It also seems against best practices to invoke return() in the middle of a CMakeLists.txt and prone to causing issues in the future if content is added after the install definitions. It would be useful if your commit message went into a bit more detail in the reason for the change and how it's beneficial.

@ouonline
Copy link
Author

What's the purpose of disabling installations? My understanding of CMake says these don't actually do anything unless you run CMake install. It also seems against best practices to invoke return() in the middle of a CMakeLists.txt and prone to causing issues in the future if content is added after the install definitions. It would be useful if your commit message went into a bit more detail in the reason for the change and how it's beneficial.

Disabling installations may be useful when a project uses Rapidjson internally and doesn't want export(or expose) Rapidjson symbols to users who use this project.

Putting return() in the middle of a CMakeLists.txt causes less diff contents but it is not a good practice here. I will push another patch for that.

can disable installations by setting `RAPIDJSON_INSTALL` to `OFF`.

Disabling installations may be useful when a project uses Rapidjson
internally and doesn't want export(or expose) Rapidjson symbols to
users who use this project.
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.

2 participants