Drop CA_RISCV_ENABLED CMake option. #684
Conversation
hvdijk
force-pushed
the
drop-riscv-enabled
branch
from
95812b7 to
f04e99b
Compare
The CA_(target)_ENABLED options were provided because we simultaneously wanted to enable all available Mux targets by default, and disable the RISC-V Mux target by default. This is confusing when CA_RISCV_ENABLED and CA_MUX_TARGETS_TO_ENABLE are mismatched: it causes the RISC-V Mux target to be disabled when either variable disables it, even when the other variable suggests that it should be enabled. Drop CA_(target)_ENABLED and rely entirely on CA_MUX_TARGETS_TO_ENABLE instead. For consistency, make the same change to CA_MUX_COMPILERS_TO_ENABLE. For both CA_MUX_TARGETS_TO_ENABLE and CA_MUX_COMPILERS_TO_ENABLE, change the default to "host", and update the documentation and continuous integration to set them where we do not want host and were not already setting them.
hvdijk
force-pushed
the
drop-riscv-enabled
branch
from
f04e99b to
c2f792a
Compare
| @@ -125,7 +125,6 @@ Currently recommended build options include: | |||
| .. code-block:: console | |||
|
|
|||
| $ cmake -GNinja \ | |||
| -DCA_RISCV_ENABLED=ON \ | |||
| -DCA_MUX_TARGETS_TO_ENABLE="riscv" \ | |||
There was a problem hiding this comment.
surely this is wrong now if we have to enable the compiler one?
I'm not totally convinced that we shouldn't pick up a default one if we have the mux one enabled, it feels quite a lot and I think it might confuse people.
| ca_option(CA_EXTERNAL_MUX_TARGET_DIRS STRING | ||
| "Semi-colon separated list of external ComputeMux target source directories" "") | ||
| ca_option(CA_MUX_COMPILERS_TO_ENABLE STRING | ||
| "Semi-colon separated list of ComputeMux compilers to enable, empty enables all" "") | ||
| "Semi-colon separated list of ComputeMux compilers to enable, empty enables all" "host") |
There was a problem hiding this comment.
could we perhaps make it so it stays "" by default, but this means match the mux targets enabled if they exist? That would be closer to what we had before.
There was a problem hiding this comment.
Also the description doesn't match the default here.
There was a problem hiding this comment.
The description says empty enables all, it doesn't say empty is the default, I think the description is right. But I like your idea of matching the Mux targets.
There was a problem hiding this comment.
yes you are right about the default, sorry about that.
Overview
Drop CA_RISCV_ENABLED CMake option.
Reason for change
The CA_RISCV_ENABLED option was provided because we simultaneously want to enable all available Mux targets by default, and disable the RISC-V Mux target by default. This is confusing when CA_RISCV_ENABLED and CA_MUX_TARGETS_TO_ENABLE are mismatched: it causes the RISC-V Mux target to be disabled when either variable disables it, even when the other variable suggests that it should be enabled.
Description of change
Drop CA_RISCV_ENABLED and rely entirely on CA_MUX_TARGETS_TO_ENABLE instead.
For consistency, make the same change to CA_MUX_COMPILERS_TO_ENABLE.
For both CA_MUX_TARGETS_TO_ENABLE and CA_MUX_COMPILERS_TO_ENABLE, change the default to "host": we expect builds that rely on other targets or other compilers to already explicitly specify the desired target and compiler.
Anything else we should know?
If there's any other relevant information we should know that may help us in
understanding and verifying your patch, please include it here.
Checklist