search

conda-forge / ctng-compiler-activation-feedstock

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

Use more standard form of release flag in `MESON_ARGS` #98

Closed h-vetinari closed 4 months ago

h-vetinari commented 1 year ago

There are two different ways to set up a release flag to meson, --buildtype release, and -Dbuildtype=release. Apparently the latter is more common; at least it's the recommended way in the docs.

Since meson-python will by default add -Dbuildtype=release, the fact that the compiler-activation does the other kind leads to a duplicate specification and forces us to strip it out again as follows (e.g. in scipy, but affects all feedstocks using meson-python).

# meson-python already sets up a -Dbuildtype=release argument to meson, so
# we need to strip --buildtype out of MESON_ARGS or fail due to redundancy
MESON_ARGS_REDUCED="$(echo $MESON_ARGS | sed 's/--buildtype release //g')"

To avoid this, use the more common form of the flag.

PS. Stumbled over this old PR, where this was discussed with @rgommers PPS. Sister PR of https://github.com/conda-forge/clang-compiler-activation-feedstock/pull/112

conda-forge-webservices[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

isuruf commented 1 year ago

This might break packages that does the opposite. So, it's not clear which one is better.

h-vetinari commented 1 year ago

This might break packages that does the opposite.

That's clear, though I think it'd be good to decide on one and try to be consistent.

So, it's not clear which one is better.

Given that meson-python uses -Dbuildtype (not only the conda-forge version, but built-in upstream), and that the meson docs use this variant too, I think the balance tilts towards that.

rgommers commented 1 year ago

Given that meson-python uses -Dbuildtype (not only the conda-forge version, but built-in upstream), and that the meson docs use this variant too, I think the balance tilts towards that.

I agree. This change is known to avoid a bunch of argparsing complexity. It is right for all Python packages. In case there are non-Python packages that have an explicit --buildtype=release in their build.sh script in conda-forge, you'll get a clear error:

ERROR: Got argument buildtype as both -Dbuildtype and --buildtype. Pick one.

You'll fix that once, which is a simple 1-line diff, and does not require argument parsing.

meson-python isn't only relevant here because it's getting more popular, but because it's an intermediate layer that adds the incompatible flag. Such a layer does not exist for other languages, so there isn't the opposite problem if we make the switch here to using -Dbuildtype.

h-vetinari commented 4 months ago

I'm adapting the changes discussed on the clang side into #121