Skip to content

Conversation

@JordanMaples
Copy link
Contributor

Reverts #964

@JordanMaples JordanMaples merged commit 25bb4bd into master Jan 4, 2021
JordanMaples referenced this pull request Jan 4, 2021
* [cmake] Adding GSL_INSTALL option

Not all consumers of GSL automatically want to have this install logic.

It's good practice to gate install logic behind an option.
For an example look at magic_enum:
https://github.com/Neargye/magic_enum/blob/master/CMakeLists.txt

If the client wants to install GSL they still can. But they should ask
for it by overriding GSL_INSTALL.

* Update cmake/guidelineSupportLibrary.cmake

added nl@eof

* Update CMakeLists.txt

* Update CMakeLists.txt

Co-authored-by: Juan Ramos <juanr0911@gmail.com>
Co-authored-by: Jordan Maples [MSFT] <49793787+JordanMaples@users.noreply.github.com>
@ghost
Copy link

ghost commented Jan 5, 2021

Was something wrong with the changes I submitted?

@JordanMaples JordanMaples deleted the revert-964-optional_install branch January 5, 2021 18:00
@JordanMaples
Copy link
Contributor Author

JordanMaples commented Jan 5, 2021

@hdf89shfdfs The problem was pointed out in the commit discussion: eca0eca
Essentially, when the install for GSL is created the Microsoft.GSLConfig.cmake file is missing the INTERFACE_INCLUDE_DIRECTORIES property.
I've been trying to find a solution, however I haven't found anything yet.

@JordanMaples
Copy link
Contributor Author

I think I found the issue. The missing line reappears when I bring the include(GNUInstallDirs) line back to the main file.

JordanMaples added a commit that referenced this pull request Jan 5, 2021
* [cmake] Adding options for INSTALL and TEST (#964)

* [cmake] Adding GSL_INSTALL option

Not all consumers of GSL automatically want to have this install logic.

It's good practice to gate install logic behind an option.
For an example look at magic_enum:
https://github.com/Neargye/magic_enum/blob/master/CMakeLists.txt

If the client wants to install GSL they still can. But they should ask
for it by overriding GSL_INSTALL.

* Update cmake/guidelineSupportLibrary.cmake

added nl@eof

* Update CMakeLists.txt

* Update CMakeLists.txt

Co-authored-by: Juan Ramos <juanr0911@gmail.com>
Co-authored-by: Jordan Maples [MSFT] <49793787+JordanMaples@users.noreply.github.com>

* missing config line restored by moving GNUInstallDirs back to main file

Co-authored-by: hdf89shfdfs <31327577+hdf89shfdfs@users.noreply.github.com>
Co-authored-by: Juan Ramos <juanr0911@gmail.com>
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