Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add shell tool clean #368

Open
wants to merge 47 commits into
base: develop
Choose a base branch
from
Open

Add shell tool clean #368

wants to merge 47 commits into from

Conversation

hiker
Copy link
Collaborator

@hiker hiker commented Nov 29, 2024

This just adds a shell tool. The tool itself is not used directly (though it could potentially replace some common tools? Not sure that we gain anything from it though :) ).

I am adding this since it is an extremely useful tool for Fab scripts. For example, many scripts might want to query nf-config for compiler- and linker flags for using NetCDF. Since it was decided not to support running Fab-triggered command using a shell (which would allow a configuration to use $(nf-config ..fflags) etc), applications now need to explicitly run a shell command.
With a shell tool in Fab (which will check various shells to find one that is available on the platform), this becomes a lot more convenient and reduces code duplication.

Unfortunately, I could not find a generic way of testing if a shell is available with a single parameter (e.g. some shells have no version command), so the most portable way I could think of was to run an echo something command ... which then requires at least two parameters (-c and echo something), requiring many changes in tool to support a list of strings instead of only a single string.

hiker and others added 30 commits September 24, 2024 17:19
Linker flags for common libraries
…ive compiler names are longer (crayftn-cray, craycc-cray).
* Moved OBJECT_ARCHIVES from constants to ArtefactSet.

* Moved PRAGMAD_C from constants to ArtefactSet.

* Turned 'all_source' into an enum.

* Allow integer as revision.

* Fixed flake8 error.

* Removed specific functions to add/get fortran source files etc.

* Removed non-existing and unneccessary collections.

* Try to fix all run_configs.

* Fixed rebase issues.

* Added replace functionality to ArtefactStore, updated test_artefacts to cover all lines in that file.

* Started to replace artefacts when files are pre-processed.

* Removed linker argument from linking step in all examples.

* Try to get jules to link.

* Fixed build_jules.

* Fixed other issues raised in reviews.

* Try to get jules to link.

* Fixed other issues raised in reviews.

* Simplify handling of X90 files by replacing the X90 with x90, meaning only one artefact set is involved when running PSyclone.

* Make OBJECT_ARCHIVES also a dict, migrate more code to replace/add files to the default build artefact collections.

* Fixed some examples.

* Fix flake8 error.

* Fixed failing tests.

* Support empty comments.

* Fix preprocessor to not unnecessary remove and add files that are already in the output directory.

* Allow find_soure_files to be called more than once by adding files (not replacing artefact).

* Updated lfric_common so that files created by configurator are written in build (not source).

* Use c_build_files instead of pragmad_c.

* Removed unnecessary str.

* Documented the new artefact set handling.

* Fixed typo.

* Make the PSyclone API configurable.

* Fixed formatting of documentation, properly used ArtefactSet names.

* Support .f and .F Fortran files.

* Removed setter for tool.is_available, which was only used for testing.

* #3 Fix documentation and coding style issues from review.

* Renamed Categories into Category.

* Minor coding style cleanup.

* Removed more unnecessary ().

* Re-added (invalid) grab_pre_build call.

* Fixed typo.

* Renamed set_default_vendor to set_default_compiler_suite.

* Renamed VendorTool to CompilerSuiteTool.

* Also accept a Path as exec_name specification for a tool.

* Move the check_available function into the base class.

* Fixed some types and documentation.

* Fix typing error.

* Added explanation for meta-compiler.

* Improved error handling and documentation.

* Replace mpiifort with mpifort to be a tiny bit more portable.

* Use classes to group tests for git/svn/fcm together.

* Fixed issue in get_transformation script, and moved script into lfric_common to remove code duplication.

* Code improvement as suggested by review.

* Fixed run config

* Added reference to ticket.

* Updated type information.

* More typing fixes.

* Fixed typing warnings.

* As requested by reviewer removed is_working_copy functionality.

* Issue a warning (which can be silenced) when a tool in a toolbox is replaced.

* Fixed flake8.

* Fixed flake8.

* Fixed failing test.

* Addressed issues raised in review.

* Removed now unnecessary operations.

* Updated some type information.

* Fixed all references to APIs to be consistent with PSyclone 2.5.

* Added api to the checksum computation.

* Fixed type information.

* Added test to verify that changing the api changes the checksum.

* Make compiler version a tuple of integers

* Update some tests to use tuple versions

* Explicitly test handling of bad version format

* Fix formatting

* Tidying up

* Make compiler raise an error for any invalid version string

Assume these compilers don't need to be hashed.
Saves dealing with empty tuples.

* Check compiler version string for compiler name

* Fix formatting

* Add compiler.get_version_string() method

Includes other cleanup from PR comments

* Add mpi and openmp settings to BuildConfig, made compiler MPI aware.

* Looks like the circular dependency has been fixed.

* Revert "Looks like the circular dependency has been fixed." ...
while it works with the tests, a real application still triggered it.

This reverts commit 150dc37.

* Don't even try to find a C compiler if no C files are to be compiled.

* Updated gitignore to ignore (recently renamed) documentation.

* Fixed failing test.

* Return from compile Fortran early if there are no files to compiles. Fixed coding style.

* Add MPI enables wrapper for intel and gnu compiler.

* Fixed test.

* Automatically add openmp flag to compiler and linker based on BuildConfig.

* Removed enforcement of keyword parameters, which is not supported in python 3.7.

* Fixed failing test.

* Support more than one tool of a given suite by sorting them.

* Use different version checkout for each compiler vendor with mixins

* Refactoring, remove unittest compiler class

* Fix some mypy errors

* Use 'Union' type hint to fix build checks

* Added option to add flags to a tool.

* Introduce proper compiler wrapper, used this to implement properly wrapper MPI compiler.

* Fixed typo in types.

* Return run_version_command to base Compiler class

Provides default version command that can be overridden for other compilers.
Also fix some incorrect tests
Other tidying

* Add a missing type hint

* Added (somewhat stupid) 'test' to reach 100% coverage of PSyclone tool.

* Simplified MPI support in wrapper.

* More compiler wrapper coverage.

* Removed duplicated function.

* Removed debug print.

* Removed permanently changing compiler attributes, which can cause test failures later.

* More test for C compiler wrapper.

* More work on compiler wrapper tests.

* Fixed version and availability handling, added missing tests for 100% coverage.

* Fixed typing error.

* Try to fix python 3.7.

* Tried to fix failing tests.

* Remove inheritance from mixins and use protocol

* Simplify compiler inheritance

Mixins have static methods with unique names,
overrides only happen in concrete classes

* Updated wrapper and tests to handle error raised in get_version.

* Simplified regular expressions (now tests cover detection of version numbers with only a major version).

* Test for missing mixin.

* Use the parsing mixing from the compiler in a compiler wrapper.

* Use setattr instead of assignment to make mypy happy.

* Simplify usage of compiler-specific parsing mixins.

* Minor code cleanup.

* Updated documentation.

* Simplify usage of compiler-specific parsing mixins.

* Test for missing mixin.

* Fixed test.

* Added missing openmp_flag property to compiler wrapper.

* Don't use isinstance for consistency check, which does not work for CompilerWrappers.

* Fixed isinstance test for C compilation which doesn't work with a CompilerWrapper.

* Use a linker's compiler to determine MPI support. Removed mpi property from CompilerSuite.

* Added more tests for invalid version numbers.

* Added more test cases for invalid version number, improved regex to work as expected.

* Fixed typo in test.

* Fixed flake/mypy errors.

* Combine wrapper flags with flags from wrapped compiler.

* Made mypy happy.

* Fixed test.

* Split tests into smaller individual ones, fixed missing asssert in test.

* Parameterised compiler version tests to also test wrapper.

* Added missing MPI parameter when getting the compiler.

* Fixed comments.

* Order parameters to be in same order for various compiler classes.

* Remove stray character

* Added getter for wrapped compiler.

* Fixed small error that would prevent nested compiler wrappers from being used.

* Added a cast to make mypy happy.

* Add simple getter for linker library flags

* Add getter for linker flags by library

* Fix formatting

* Add optional libs argument to link function

* Reorder and clean up linker tests

* Make sure `Linker.link()` raises for unknown lib

* Add missing type

* Fix typing error

* Add 'libs' argument to link_exe function

* Try to add documentation for the linker libs feature

* Use correct list type in link_exe hint

* Add silent replace option to linker.add_lib_flags

* Fixed spelling mistake in option.

* Clarified documentation.

* Removed unnecessary functions in CompilerWrapper.

* Fixed failing test triggered by executing them in specific order (tools then steps)

* Fixed line lengths.

* Add tests for linker LDFLAG

* Add pre- and post- lib flags to link function

* Fix syntax in built-in lib flags

* Remove netcdf as a built-in linker library

Bash-style substitution is not currently handled

* Configure pre- and post-lib flags on the Linker object

Previously they were passed into the Linker.link() function

* Use more realistic linker lib flags

* Formatting fix

* Removed mixing, use a simple regex instead.

* Added support for ifx/icx compiler as intel-llvm class.

* Added support for nvidia compiler.

* Add preliminary support for Cray compiler.

* Added Cray compiler wrapper ftn and cc.

* Made mpi and openmp default to False in the BuildConfig constructor.

* Removed white space.

* Follow a more consistent naming scheme for crays, even though the native compiler names are longer (crayftn-cray, craycc-cray).

* Changed names again.

* Support compilers that do not support OpenMP.

* Added documentation for openmp parameter.

* Renamed cray compiler wrapper to be CrayCcWrapper and CrayFtnWrapper, to avoid confusion with Craycc.

* Fixed incorrect name in comments.

---------

Co-authored-by: jasonjunweilyu <161689601+jasonjunweilyu@users.noreply.github.com>
Co-authored-by: Luke Hoffmann <luke.hoffmann@bom.gov.au>
Co-authored-by: Luke Hoffmann <992315+lukehoffmann@users.noreply.github.com>
@hiker hiker requested a review from jasonjunweilyu November 29, 2024 06:44
Copy link
Collaborator

@MatthewHambley MatthewHambley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key issue with this change is whether we should try to support multiple shells or just the Posix standard shell. There is potential to simplify the change if we support only the standard shell and I'm not sure we get any advantage for supporting others.

source/fab/tools/shell.py Show resolved Hide resolved
source/fab/tools/tool.py Outdated Show resolved Hide resolved
source/fab/tools/tool_repository.py Outdated Show resolved Hide resolved
source/fab/tools/tool_repository.py Show resolved Hide resolved
tests/unit_tests/steps/test_grab.py Show resolved Hide resolved
tests/unit_tests/tools/test_shell.py Outdated Show resolved Hide resolved
@hiker
Copy link
Collaborator Author

hiker commented Dec 3, 2024

@MatthewHambley, thanks for your comments. Notice that this PR is a bit in 'your future', it's 4 prs ahead of main, which is why I had it marked for Jason ;)

Not sure how you would like to proceed. I won't try to merge the PRs in out-of-order - I've spent way too much time when I migrated our branches across (admittedly because of user error ... I mixed up the order of two branches ;) ). So you can either continue, and once this is OK, I'll keep it in its position, and we just merge it when we reach it. Or you wait till I 'trickle' this PR through to you as the next one, and I'll let Jason finish this for now (i.e. for a merge into develop)

@hiker hiker requested a review from MatthewHambley December 4, 2024 07:20
@hiker
Copy link
Collaborator Author

hiker commented Dec 9, 2024

Ready for next review.

Copy link
Collaborator

@jasonjunweilyu jasonjunweilyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hiker , this PR is fine for me. I only have a minor comment. I think this addition of shell tool adds an additional check for user's environment and may help at some stage for signalling incorrect environment configurations.

source/fab/tools/tool.py Show resolved Hide resolved
@hiker hiker requested a review from jasonjunweilyu December 12, 2024 04:56
Copy link
Collaborator

@jasonjunweilyu jasonjunweilyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now ready to be merged into develop branch. The shell tool addition may help with debugging for user environment when something goes really wrong.

Copy link
Collaborator

@MatthewHambley MatthewHambley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this change under protest.

@hiker
Copy link
Collaborator Author

hiker commented Dec 24, 2024

I have checked, all applications we have at are de-facto sh script, so since you are really concerned, we can remove bash etc from the loop, so only sh gets created. Note that I still want the test that sh works, just to be consistent among all tools, and give better error messages to the user (i.e. 'sh' is not available, as opposed a potential cryptic error message when the user tries to run a script, which then might not work).
Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants