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

use system library, if GIT_SUBMODULE=OFF #1907

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

dschwoerer
Copy link
Contributor

No description provided.

@ZedThree
Copy link
Member

This is failing because we've not yet checked out the submodules, so it tries to use the (non-existent) system versions of mpark/fmt.

As it is, this essentially makes using system versions the default on first checkout, unless the user has cloned recursively (which is not the default behaviour of git clone). This isn't really what we want.

What's the use case you're thinking of for this change?

@dschwoerer
Copy link
Contributor Author

The use case would that if the user explicitly sets GIT_SUBMODULE=OFF and the submodules aren't there, that then the system ones are used.
I think it should have been:

if(GIT_SUBMODULE OR (EXISTS externalpackages/mpark.variant/CMakeLists.txt))
  option(BOUT_USE_SYSTEM_MPARK_VARIANT "Use external installation of mpark.variant" OFF)
else()
  option(BOUT_USE_SYSTEM_MPARK_VARIANT "Use external installation of mpark.variant" ON)
endif()

@ZedThree
Copy link
Member

ZedThree commented Feb 4, 2020

Ok, I see now. Still, I'm not sure of the advantage of this over setting BOUT_USE_SYSTEM_* explicitly, aside from being a little bit less typing. Setting GIT_SUBMODULE=OFF doesn't really mean "don't use the submodules", it means "don't update the submodules".

What if we made the error more explicit: "mpark_variant not found. Either set GIT_SUBMODULE=ON or use a system version (BOUT_USE_SYSTEM_MPARK_VARIANT=ON)"? Perhaps also a better name for GIT_SUBMODULE, BOUT_UPDATE_GIT_SUBMODULES?

Alternatively, having a BOUT_USE_SYSTEM_LIBRARIES option and using
CMakeDependentOption to set the specific BOUT_USE_SYSTEM_* variables. Note that this will correctly update the dependent options if BOUT_USE_SYSTEM_LIBRARIES changes value.

@dschwoerer
Copy link
Contributor Author

Sure, but if they are nether present, nor the user don't want to install them, I thought it makes sense to check whether they might be present. I don't have a strong opinion, and am happy to explicitly pass the flags if needed. But I thought it might be nicer to do that automatically, i.e. less typing and just do the right thing.

I found the aggressive caching from cmake very annoying, and always rm -rf * before running cmake .. again, so don't know about those caching issues.

Anyway. I am happy if you close this, just thought it might be neat.

@ZedThree ZedThree merged commit a0bb2dd into optional-external-mpark Mar 5, 2020
@ZedThree ZedThree deleted the optional-external-mpark-auto branch March 5, 2020 11:39
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.

None yet

2 participants