search

conda-forge / ctng-compiler-activation-feedstock

A conda-smithy repository for ctng-compiler-activation.
BSD 3-Clause "New" or "Revised" License
13 stars 22 forks source link

Why isn't `CMAKE_BUILD_TYPE=Release` included in `CMAKE_ARGS`? #94

Closed jdblischak closed 1 year ago

jdblischak commented 1 year ago

Comment:

I recently started using the auto-defined CMAKE_ARGS environment variable for my CMake-based conda builds, as suggested in the conda-forge docs on using CMake. Not only is it convenient, but I agree with the perspective in https://github.com/conda-forge/ctng-compiler-activation-feedstock/issues/77#issuecomment-1145159371 that using shared CMake settings helps keep the conda-forge ecosystem consistent.

However, I was recently surprised to learn that CMAKE_BUILD_TYPE=Release is not set by activate-gcc.sh in this repo (https://github.com/conda-forge/staged-recipes/pull/23297). It is set by activate-clang.sh for osx builds and by activate.bat for MSVC builds on Windows. I wasn't able to find any discussion on the motivation for this inconsistency across the platforms. In fact, all I could find was a discussion that was debating setting CMAKE_BUILD_TYPE=None (#87, https://github.com/conda-forge/conda-forge.github.io/issues/1859).

Is there a particular motivation why CMAKE_BUILD_TYPE=Release is the default for osx and win builds, but not linux? If not, can I send I PR to include CMAKE_BUILD_TYPE=Release in CMAKE_ARGS?

isuruf commented 1 year ago

If not, can I send I PR to include CMAKE_BUILD_TYPE=Release in CMAKE_ARGS?

Yes, please

jakirkham commented 1 year ago

What would be the path to override this if a user wants a debug build of a package?

isuruf commented 1 year ago

Instead of

cmake ${CMAKE_ARGS} .

you do

cmake ${CMAKE_ARGS} -DCMAKE_BUILD_TYPE=Debug .
jakirkham commented 1 year ago

It's ok to specify CMAKE_BUILD_TYPE twice?

cc @robertmaynard

robertmaynard commented 1 year ago

It's ok to specify CMAKE_BUILD_TYPE twice?

cc @robertmaynard

Yes. The last value will be used.

pitrou commented 11 months ago

Why has this been done? It has started breaking CI builds for us in Arrow. The build type is a developer-oriented setting, not something that Conda should dictate to users. For example, it is often recommended to build in Debug mode for development.

pitrou commented 11 months ago

Not only is it convenient, but I agree with the perspective in #77 (comment) that using shared CMake settings helps keep the conda-forge ecosystem consistent.

I think this conflates two different things:

There really should be two different environment variables for those. For example CMAKE_ARGS and CMAKE_ARGS_FOR_CONDA_BUILD.

xhochy commented 11 months ago

We could set some variables likeBUILD_TYPE conditionally on the activation being run inside of CONDA_BUILD. We already have a conditional block for this in https://github.com/conda-forge/ctng-compiler-activation-feedstock/blob/688de6d31129e584a5f164a28c50806353056c63/recipe/activate-gcc.sh#L86

jdblischak commented 11 months ago

It has started breaking CI builds for us in Arrow.

@pitrou I'm sorry this has caused an issue for you. I appreciate that unexpected build failures are always frustrating.

Why has this been done?

The motivation is two-fold:

There really should be two different environment variables for those. For example CMAKE_ARGS and CMAKE_ARGS_FOR_CONDA_BUILD.

Could you please explain why the workaround noted above isn't ideal for your setup? If you know you need a specific build type, you can include it in the call to cmake, and the value set in this repository is overridden. For example, if your CI builds use cmake ${CMAKE_ARGS} -DCMAKE_BUILD_TYPE=Debug ., then they will be unaffected by what conda-forge decides to set as its default.

pitrou commented 11 months ago

Could you please explain why the workaround noted above isn't ideal for your setup?

It certainly works. It's just that 1) it's more fragile (you have to make sure you use the correct parameter order in your cmake invocation) 2) the change broke things for us in a rather gratuitous way, and without warning.

Certainly, regardless of the final decision made by conda-forge, we can easily work around it... However, in general, I don't think conda/conda-forge should change compilation defaults except where necessary (such as setting the correct library search path).

A general request: please think about user experience instead of doing unexpected changes like that. Also, I would suggest looking at what other package repositories do, because it can serve as a reference for good packaging practices. If Ubuntu/Fedora/etc. don't force the default CMake build type, why would conda-forge behave differently?

pitrou commented 11 months ago

(in our particular case, "broke things for us in a rather gratuitous way" implies that we had 1) to research the underlying cause of our CI issues 2) try a workaround and schedule a general CI rebuild to ensure it doesn't break something else; that's easily one or two hours lost, not to mention the annoyance of unrelated CI failures on pending PRs)