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

[cmake] Adding options for INSTALL and TEST #964

Merged
merged 4 commits into from Jan 4, 2021
Merged

[cmake] Adding options for INSTALL and TEST #964

merged 4 commits into from Jan 4, 2021

Conversation

ghost
Copy link

@ghost ghost commented Dec 29, 2020

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.

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.
@ghost
Copy link
Author

ghost commented Dec 31, 2020

@JordanMaples do the changes look acceptable?

@JordanMaples
Copy link
Contributor

At a quick glance everything looks good. I'll do a deeper dive in this on Monday.

Does adding the project options change how cmake needs to be invoked in the CLI? If so, would you mind adding a blurb in the readme about invoking these options.

@ghost
Copy link
Author

ghost commented Jan 1, 2021

At a quick glance everything looks good. I'll do a deeper dive in this on Monday.

Does adding the project options change how cmake needs to be invoked in the CLI? If so, would you mind adding a blurb in the readme about invoking these options.

It shouldn't affect the CLI

@JordanMaples JordanMaples changed the title [cmake] Adding GSL_INSTALL option [cmake] Adding options for INSTALL and TEST Jan 4, 2021
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@JordanMaples
Copy link
Contributor

Maintainer's call: Merging this. I verified the changes locally and everything looks good to me. Thanks!

@JordanMaples JordanMaples merged commit eca0eca into microsoft:master Jan 4, 2021
JordanMaples added a commit that referenced this pull request Jan 4, 2021
JordanMaples added a commit that referenced this pull request Jan 4, 2021
JordanMaples added a commit to JordanMaples/GSL that referenced this pull request Jan 5, 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>
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.

1 participant