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

Prefix cmake options with CORRADE_ #139

Closed
jonesmz opened this issue May 29, 2022 · 4 comments
Closed

Prefix cmake options with CORRADE_ #139

jonesmz opened this issue May 29, 2022 · 4 comments

Comments

@jonesmz
Copy link

jonesmz commented May 29, 2022

My project has many 3rd party libraries in the same build tree. I just realized that some options from other third party libs are also being used by Corrade, and it's resulting in some frustrations.

If the various options that can be set in the Corrade CMakeLists.txt files were prefixed with CORRADE_ there would be no name collision.

@mosra
Copy link
Owner

mosra commented May 29, 2022

Yes, this is on my list -- mosra/magnum#453.

I feel your pain, I just didn't have the courage to start yet because the downstream breakages will be serious if I don't provide backwards compatibility... and it won't really solve the name collisions if I provide backwards compatibility.

@mosra mosra added this to the 2022.0a milestone May 29, 2022
@mosra mosra added this to TODO in Project management via automation May 29, 2022
@jonesmz
Copy link
Author

jonesmz commented May 29, 2022

You could keep the existing option names with the following strategy:

  1. Create a new option CORRADE_DISABLE_CMAKE_BACKCOMPAT, or similar, default to ON.
  2. If CORRADE_DISABLE_CMAKE_BACKCOMPAT is not set to OFF, then 3.
  3. if 2, then for each existing option, if the CORRADE_ version is not set, then set(CORRADE_${optname} ${optname})

This would allow projects that understand the new symbols names to not have conflicts, and projects that do not understand them keep the existing behavior.

Then some day in the future, you change the default of CORRADE_DISABLE_CMAKE_BACKCOMPAT from ON to OFF, and eventually remove it and the old names.

@mosra
Copy link
Owner

mosra commented May 29, 2022

Hm, I actually had a similar idea before but there were some loose ends:

  • There's now a BUILD_DEPRECATED option (enabled by default), which would become CORRADE_BUILD_DEPRECATED (enabled by default).
  • If CORRADE_BUILD_DEPRECATED is enabled, the old unprefixed names are still recognized, but with a deprecation warning. BUILD_DEPRECATED doesn't affect this.
  • Projects that disable only BUILD_DEPRECATED would still have the old unprefixed names recognized (because otherwise everything would break for everyone)
  • Projects that disable CORRADE_BUILD_DEPRECATED would not have the old unprefixed names recognized anymore, thus preventing conflicts. But this also affects deprecations in the C++ APIs, meaning people get either no backwards compatibility at all, or backwards compatibility including the unprefixed CMake names.

Yeah, and adding a dedicated option just for this one CMake change feels dirty to me. -DCORRADE_PLEASE_BEHAVE_THANKS_VERY_MUCH=ON. Not a fan of flags that people have to learn to enable to get reasonable defaults.

Oh, or I could detect if any new prefixed CORRADE_ options are set, and then assume the user is aware of the prefixed options aready and not even provide the backwards compat. But that's also not 100% bulletproof, because (and especially with Corrade, as opposed to Magnum), the default set of options is usually fine and nobody needs to enable/disable anything.... so you'd have to explicitly do something trivial like -DCORRADE_WITH_UTILITY=ON to cause the backwards compat to get disabled.

@mosra
Copy link
Owner

mosra commented Jun 12, 2022

This should be implemented as of 878624a, with similar commits gradually appearing in other repos. See the commit message for details.

There's quite a bit of nontrivial logic to both satisfy backwards compatibility and allow the unprefixed variables to be repurposed by other projects, and while I tried to verify all corner cases, it might not be covering everything, especially the more obscure combinations of options. Please report if it misbehaves in some way -- thanks! :)

@mosra mosra closed this as completed Jun 12, 2022
Project management automation moved this from TODO to Done Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants