search

metomi / fab

Flexible build system for scientific software
https://metomi.github.io/fab/
Other
19 stars 11 forks source link

3 better compiler support #309

Closed hiker closed 1 week ago

hiker commented 2 weeks ago

This is the big one - it introduces Tool as baseclass for anything called via subprocess (and removed run_command therefore). It introduces the ToolRepository and ToolBox. Best to read the documentation (atm site-specific-configuration.rst).

Note that this is not really complete, outstanding issues are support for compiler-specific flags, better support for linking, handling openmp (so that we can trigger fparser's OMP sentinel handling instead of using a work around).

We did fix up the run_configs, but they won't really work better than before (as in, still hard-coded compiler etc). Once we have a fully working LFRic build system (including the above todos), we will update all the run configs.

hiker commented 1 week ago

Thanks a lot for the review. I believe I have addressed most issues. Outstanding:

  1. I am not sure about fixing the typing - if I fix it, I need to use quite a few casts. I think that might be ok, but if you have a better idea please let me know.
  2. Proper handling of compiler wrapper. I started to fix it up here, but I think that becomes to big and should better be done standalone later.
  3. Improved linker handling - it's still a bit odd.
  4. I need some feedback re adding new (user defined) tools (#311): should I merge all tool categories into a generic 'misc' one, which will make it a lot easier for the user to add new tools? To me that feels sufficient (though that means that many generic tools like rsync, svn, git, ... will be found by name, instead of the corresponding category. I think that's fine, since even if a site has a different named git version, we can still just add this as a separate tool - using the 'standard' name (say git), but calling git-3.1415 as executable name. Just remove the old git, and replace it with a new one.
  5. Default intel compiler in run_configs: while I could fix this now, it would lead to a lot of code duplication. I propose to leave this for now, once we have all other things fixed, I believe it will be a lot less code duplication (e.g. if the ToolRepository can check to automatically find the right mpif90 version of a specified compiler etc).
  6. I've lost the comment atm - while I did not use which for tool availability (since using a specific command has the advantage that it tests things like ld_library_path), I moved this handling into the base class, which removes a lot of duplicate code (and still allows to customise the command to use per tool). I did use which in one of the run_configs example.

I believe there might be another issue or two that I should mentioned here, but I believe I have comments on all your feedback, so it will be in mu answers. I've opened a few tickets (#311, #312, #313, #314) to keep track of follow-up work.

Thanks for the review

hiker commented 1 week ago

OK, I believe I have addressed all issues, and as you suggested I closed all conversations where I think we have agreed :) I left a few things open where I think you should provide feedback to confirm :)