Skip to content

[cmake] Adding options for INSTALL and TEST#964

Merged
JordanMaples merged 4 commits into
masterfrom
unknown repository
Jan 4, 2021
Merged

[cmake] Adding options for INSTALL and TEST#964
JordanMaples merged 4 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Dec 29, 2020

Copy link
Copy Markdown

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

ghost commented Dec 31, 2020

Copy link
Copy Markdown
Author

@JordanMaples do the changes look acceptable?

@JordanMaples

Copy link
Copy Markdown
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.

Comment thread cmake/guidelineSupportLibrary.cmake Outdated
@ghost

ghost commented Jan 1, 2021

Copy link
Copy Markdown
Author

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
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
@JordanMaples

Copy link
Copy Markdown
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