search

seL4 / sel4test

Test suite for seL4.
http://sel4.systems
Other
24 stars 60 forks source link

Inform the user that their settings might be getting overridden #29

Open Jesse-Millwood opened 3 years ago

Jesse-Millwood commented 3 years ago

It would be nice to notify the developer that some of their setting might get overridden. I have personally been burned by this many times, where I would like to create a binary and set it in the easy-settings but just to find that it was silently overridden as an elf. Often times, a bootloader won't tell me what is wrong and I usually spend more time than I'd like to admit trying to find what the issue is. I understand where setting some sane defaults by be helpful across projects. Is there a way in CMake to check if a variable has been set by the user or if it was set by the default value of config_string?

I'm not sure where the best place is but some possibilities might be the conditionals that check it [1][2] or the Data61 override function [3]. It might be better to check at each variable that is being set in the override functions too. I'm not sure what the best course of action would be.

1: https://github.com/seL4/sel4test/blob/7b1f1fcb0bf9625766c52c149ded5abe9c830ca5/settings.cmake#L50 2: https://github.com/seL4/sel4test/blob/90a835e26c93e956b8f4b0893a20458b41ff71a0/CMakeLists.txt#L31 3: https://github.com/seL4/seL4_tools/blob/ffe3305d8d3926ccfa0fa8019063c786b75850d6/cmake-tool/helpers/application_settings.cmake#L10

kent-mcleod commented 3 years ago

Are you thinking of something like emitting a warning message every time a CMake cache variable is changed? This would involve adding a check to every FORCE or INTERNAL CMake setting operation and printing out a message every time that the option was previously set and is being changed to a different value. This makes sense to me as a general solution.

Specifically for settings related to boot loaders, there's probably a missing mechanism to provide a settings file that localizes boot loader settings. Probably something like a CMAKE_TOOLCHAIN_FILE (https://cmake.org/cmake/help/latest/variable/CMAKE_TOOLCHAIN_FILE.html) but for boot loader settings. Then the ApplyData61ElfLoaderSettings could be used as the default while people with different setups can provide their own.

Jesse-Millwood commented 3 years ago

I guess I was mainly talking about the few settings that are overridden but I like your idea about checking in general much more. Could there be a wrapper around set, that does the checking?

That toolchain file is a good idea too and would help keep things separated.