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

Make RapidJSON_INCLUDE_DIR non-blank in Config.cmake #1079

Merged
merged 2 commits into from Oct 9, 2017

Conversation

Projects
None yet
3 participants
@captaincrutches
Contributor

captaincrutches commented Oct 3, 2017

If I want to use RapidJSON as a submodule for my own project, the currently output RapidJSONConfig.cmake has a blank string for RapidJSON_INCLUDE_DIR, making it necessary to include its headers indirectly (in my case, with ${RapidJSON_SOURCE_DIR}/include). This change simply sets a value for those variables, so I can use my submodule more easily.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 3, 2017

Coverage Status

Coverage remained the same at 99.918% when pulling 84ca485 on captaincrutches:cmake-include-dir into 2a0bc60 on Tencent:master.

coveralls commented Oct 3, 2017

Coverage Status

Coverage remained the same at 99.918% when pulling 84ca485 on captaincrutches:cmake-include-dir into 2a0bc60 on Tencent:master.

@miloyip

This comment has been minimized.

Show comment
Hide comment
@miloyip

miloyip Oct 9, 2017

Collaborator

I am not very familiar with CMake actually. Will this affect other current users?

Collaborator

miloyip commented Oct 9, 2017

I am not very familiar with CMake actually. Will this affect other current users?

@captaincrutches

This comment has been minimized.

Show comment
Hide comment
@captaincrutches

captaincrutches Oct 9, 2017

Contributor

Unless current users rely on RapidJSON_INCLUDE_DIR being blank in the build tree, I don't see why it would... but if someone knows of a use case like that, I'd be interested to hear it. This shouldn't affect the install tree if I'm reading the file correctly, so only users who are actually building from source and looking for the package in the source directory (like in a submodule) should see this change. A make install just copies the RapidJSONConfig.cmake from the install tree, which doesn't get hit by this.

Contributor

captaincrutches commented Oct 9, 2017

Unless current users rely on RapidJSON_INCLUDE_DIR being blank in the build tree, I don't see why it would... but if someone knows of a use case like that, I'd be interested to hear it. This shouldn't affect the install tree if I'm reading the file correctly, so only users who are actually building from source and looking for the package in the source directory (like in a submodule) should see this change. A make install just copies the RapidJSONConfig.cmake from the install tree, which doesn't get hit by this.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 9, 2017

Coverage Status

Coverage increased (+2.0e-05%) to 99.918% when pulling 4952662 on captaincrutches:cmake-include-dir into 2a0bc60 on Tencent:master.

coveralls commented Oct 9, 2017

Coverage Status

Coverage increased (+2.0e-05%) to 99.918% when pulling 4952662 on captaincrutches:cmake-include-dir into 2a0bc60 on Tencent:master.

@miloyip miloyip merged commit d71ad00 into Tencent:master Oct 9, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
coverage/coveralls Coverage increased (+2.0e-05%) to 99.918%
Details
@miloyip

This comment has been minimized.

Show comment
Hide comment
@miloyip

miloyip Oct 9, 2017

Collaborator

OK. Thanks

Collaborator

miloyip commented Oct 9, 2017

OK. Thanks

@captaincrutches captaincrutches deleted the captaincrutches:cmake-include-dir branch Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment