Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Make fypp install/export optional #35

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

awnawab
Copy link
Contributor

@awnawab awnawab commented Feb 23, 2024

Currently, fckit will override any local fypp if it is included in a bundle. This can be problematic if a project in that bundle needs a newer version of fypp. This PR changes the default behaviour so that fypp is only installed and exported by fckit if it is not already found.
Could you please tag a release once this PR is approved and merged? I would like to include it in an updated ifs-bundle.

@FussyDuck
Copy link

FussyDuck commented Feb 23, 2024

CLA assistant check
All committers have signed the CLA.

@wdeconinck wdeconinck force-pushed the feature/fypp-optional-install branch from c2f193e to 54feb73 Compare February 23, 2024 18:27
@wdeconinck wdeconinck added approved-for-ci Approved for CI run and removed contributor labels Feb 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.07%. Comparing base (a92be06) to head (f49879d).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop      #35   +/-   ##
========================================
  Coverage    52.07%   52.07%           
========================================
  Files           52       52           
  Lines         4388     4388           
========================================
  Hits          2285     2285           
  Misses        2103     2103           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wdeconinck
Copy link
Member

This currently fails the downstream-ci:

/usr/bin/python3.9: can't open file '/opt/actions-runner/work/_work/_temp/install/fckit/libexec/fckit-fypp.py': 
[Errno 2] No such file or directory

CMakeLists.txt Outdated
install( FILES contrib/fypp-3.0-7895a7e-20200112/bin/fypp DESTINATION libexec RENAME fckit-fypp.py PERMISSIONS ${install_permissions} )
if( NOT FYPP )
install( FILES contrib/fypp-3.0-7895a7e-20200112/bin/fypp DESTINATION libexec RENAME fckit-fypp.py PERMISSIONS ${install_permissions} )
endif()
Copy link
Member

Choose a reason for hiding this comment

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

We need to (re)install the externally found fypp as well, so that downstream packages using fckit's macro can use it as well.

@github-actions github-actions bot removed the approved-for-ci Approved for CI run label Feb 27, 2024
@wdeconinck wdeconinck added the approved-for-ci Approved for CI run label Feb 27, 2024
@awnawab awnawab force-pushed the feature/fypp-optional-install branch from c4fde5c to f49879d Compare February 27, 2024 10:26
@github-actions github-actions bot removed the approved-for-ci Approved for CI run label Feb 27, 2024
@wdeconinck wdeconinck added the approved-for-ci Approved for CI run label Feb 27, 2024
@wdeconinck wdeconinck force-pushed the feature/fypp-optional-install branch from f49879d to 2dc0bd8 Compare February 27, 2024 10:51
@github-actions github-actions bot removed the approved-for-ci Approved for CI run label Feb 27, 2024
@wdeconinck wdeconinck merged commit daf91d7 into ecmwf:develop Feb 27, 2024
6 checks passed
@wdeconinck
Copy link
Member

@awnawab version 0.11.1 contains this change.

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

Successfully merging this pull request may close these issues.

4 participants