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

Fypp call update for windows compatability #742

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chuckyvt
Copy link
Contributor

This is a small change I made to the fypp call in CMake so that it can run fypp and build on Windows. If CMake can't find fypp the location can be specified via '-DCMAKE_INCLUDE_PATH" variable when configuring project.

Updated top level CMakeLists to explicitly call the python interpreter when running fypp.
Added back fypp help string
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you for adding this check. However, it breaks the CI. Any idea why?

@@ -42,12 +42,18 @@ if(NOT DEFINED CMAKE_MAXIMUM_RANK)
set(CMAKE_MAXIMUM_RANK 4 CACHE STRING "Maximum array rank for generated procedures")
endif()

# --- find preprocessor
find_program(FYPP fypp)
# Find python. Python 3.5 or higher required for ftypp
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Find python. Python 3.5 or higher required for ftypp
# Find python. Python 3.5 or higher required for fypp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CMake wasn't find fypp. I had assumed (incorrectly) that the find_file function would find the fypp package if it had been installed. Doing some trials, its seem it will be difficult to reliably find fypp across the various installation options, without the user manually specifying the path via -DCMAKE_INCLUDE_PATH or similar.

Based on that, I think its worth considering including fypp in the stdlib package. Its a single py file, so relatively compact. and the fypp license allows it. The pull request has been updated to reflect this.

Understand if don't want to add, in that case I can close the pull request.

@jvdp1
Copy link
Member

jvdp1 commented Dec 26, 2023

@awvwgk any ideas on how to solve this issue? Maybe a solution like for test-drive (with CMake files)?

I am not in favour of adding the fypp file into the stdlib project.

@jvdp1 jvdp1 requested review from awvwgk and a team December 26, 2023 19:51
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

I would prefer to download fypp from the repo or pypi rather than committing it to our repository.

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.

3 participants