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

Initial Windows Support for ENDFtk #217

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

Conversation

HunterBelanger
Copy link

This PR makes a few modifications so that ENDFtk can be compiled natively on Windows. For now, the changes have been quite minor:

  • Use development branch of NJOY tools to avoid problems with auto returns in the logger
  • Change two methods inside InterpolationBase to avoid MSVC compiler errors due to calling previously defined auto return methods
  • Change a CMake setting to avoid MSVC complaining about unknown compiler flags
  • Update two unit tests that were failing due to iterator invalidation

After making these changes, all the tests passed on my system (Windows 11 with MSVC 19.40). There are still, however, a few issues to address:

  • Windows and CMake were constantly complaining about file paths which are too long (almost exclusively in the unit tests). Because of this, some of the tests just wouldn't even be built. To fix this, I had to update my Windows registry to allow longer file names. This allowed those tests to be built, and the passed, but CMake and Windows is still complaining about paths that are too long and can't be tracked.
  • Initially, two unit tests were failing. After examination, it appeared that this was likely due to iterator invalidation when adding strings together in the test. Fixing this which allowed the tests to pass. I have not checked to see if this type of problem exists in other tests though, so this might be something that needs to be addressed elsewhere.
  • For now, I have just set the dependencies to pull the development branch of NJOY tools. I made a parallel PR there to fix the unknown compiler flag warnings in that package. At some point, we will need to set this dependency to a commit hash.

@whaeck I am now sure how you want to address these remaining problems (primarily the first one) ?

@whaeck
Copy link
Member

whaeck commented Feb 4, 2025

@HunterBelanger We're working on this internally and setting up a Windows environment to be able to compile on Windows with MSVC so that we can do some checks.

@HunterBelanger
Copy link
Author

For the moment, I have only been testing on MSVC, as using Intel on Windows leads to pain with Python, as I need to tell the interpreter that it is allowed to load all the intel shared libraries first. Is checking the Intel chain something that you would like with this though ?

Also, I know that you updated the current release version for njoy/tools, but I have been waiting to see how you want to handle this PR before updating the commit hash for that dependency.

@whaeck
Copy link
Member

whaeck commented Feb 4, 2025

At the very least we want to get MSVC to be up and running on Windows. We can look into the intel issue once that is done.

We have an internal branch with most of the changes of this PR in it (as I mentioned above, we're left with the InterpolationBase auto issue). Once these changes go in develop internally, we can close this PR.

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.

None yet

2 participants