Skip to content

Fypp robustness#53

Draft
marsdeno wants to merge 2 commits intoecmwf:developfrom
marsdeno:fypp_robustness
Draft

Fypp robustness#53
marsdeno wants to merge 2 commits intoecmwf:developfrom
marsdeno:fypp_robustness

Conversation

@marsdeno
Copy link
Contributor

Approach that allowed to get around path length and venv handling issues in easybuild-based installation of fckit.

* Without this export, in some cases wrong python interpreter is found.
* remove superfluous arguments, and remove unnecessary fckit-eval script
  leading to much shorter command string
* without this, custom_command string length problems can occur
@FussyDuck
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

set( _fckit_eval_script ${fckit_BASE_DIR}/libexec/fckit-eval.sh )
set( FCKIT_VENV_EXE ${fckit_BASE_DIR}/@rel_venv_exe_path@ )
set( FYPP ${_fckit_eval_script} ${FCKIT_VENV_EXE} -m fypp )
set( FYPP ${FCKIT_VENV_EXE} -m fypp )
Copy link
Member

Choose a reason for hiding this comment

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

The fckit_eval_script is necessary, because it allowed to exclude some arguments:
#23

Copy link
Contributor

@awnawab awnawab Dec 4, 2025

Choose a reason for hiding this comment

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

Which downstream app was the eval script causing a problem for? I solved a similar problem for FIELD_API in the past, so I might be able to help.

list( APPEND _args "$<$<BOOL:${prop}>:-D $<JOIN:${prop}, -D >>" )
endforeach()
endif()
endif()
Copy link
Member

Choose a reason for hiding this comment

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

This code is required to automatically give the fypp preprocessor access to the target compile definitions and target include directories.
What is the issue w.r.t. robustness here?

set( options NO_LINE_NUMBERING )
set( single_value_args "" )
set( multi_value_args FYPP_ARGS FYPP_ARGS_EXCLUDE DEPENDS )
set( multi_value_args FYPP_ARGS FYPP_ARGS_EXCLUDE DEPENDS INCLUDEDIRS )
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with ecbuild_add_executable we can name this INCLUDES and also add DEFINITIONS

                        [ INCLUDES <path1> [<path2> ...] ]
                        [ DEFINITIONS <definition1> [<definition2> ...] ]

set( options )
set( single_value_args TARGET )
set( multi_value_args )
set( multi_value_args INCLUDEDIRS )
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with ecbuild_add_test we should use INCLUDES, and they should already be part of the signature of add_fctest which just extends ecbuild_add_test.

@ecmwf ecmwf deleted a comment from github-actions bot Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments