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

Updates for building with NCrystal support (and fix CI) #3274

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

tkittel
Copy link
Contributor

@tkittel tkittel commented Jan 23, 2025

Description

This PR has two NCrystal related fixes, which ensures:

  1. The code will keep building once NCrystal 4.0.0 is out in a few weeks (it will remain compatible with older NCrystal releases, the fix is simply to remove a spurious include statement which was never needed).
  2. That the CMake layer can find NCrystal, even when NCrystal was installed via a Python tool like pip.

I also made a minimal related documentation change, since NCrystal should in general be installed from conda or pip these days, so there is no need for any post-install setup hassle.

This issue does not fix any open issues, since NCrystal release 4.0.0 is not out yet :-)

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

(I checked everything, but of course a few were not applicable).

@tkittel tkittel marked this pull request as draft January 23, 2025 09:02
@tkittel
Copy link
Contributor Author

tkittel commented Jan 23, 2025

The cpp-linter failure must surely be unrelated to this PR, since all I did was to remove a single include statement!

@ebknudsen
Copy link
Contributor

@tkittel - I took a look at the raw logs - it appears that the problem is in ll:60
2025-01-23T09:02:09.7763192Z ##[notice]File include/openmc/ncrystal_interface.h does not conform to Custom style guidelines. (lines 60, 61, 62, 64, 65, 66, 69, 70, 71)
You didn't change those lines so the linter directives have probably changed. Don't know what the problem is though.

@tkittel
Copy link
Contributor Author

tkittel commented Jan 23, 2025

@ebknudsen yeah thanks, that's how far I got as well :-)

If there was some sort of hint of what needs to be changed, I would not mind to go and do a quick fix. However, it really is unrelated to this PR, and I would prefer to handle that in a separate PR.

@tkittel
Copy link
Contributor Author

tkittel commented Feb 3, 2025

Well, I took a look at the workflow and figured out that one can try to run clang-format --style=file <somefile.h> to see what it has to be like. So I fixed the linter, and also rebased for good measure.

@tkittel
Copy link
Contributor Author

tkittel commented Feb 10, 2025

@pshriwise so I added a fix now for the CI like mentioned on #3269 .

I am in principle on vacation now, but if the CI all checks out in this PR, I recommend merging it to fix your CI :-)

(if it does NOT all check out, I will try to fix it)

@tkittel
Copy link
Contributor Author

tkittel commented Feb 10, 2025

Looks like a few more issues to resolve, trying to have a look.

@tkittel tkittel force-pushed the develop branch 2 times, most recently from f26efd2 to 53e2491 Compare February 11, 2025 09:05
@tkittel tkittel marked this pull request as ready for review February 11, 2025 13:04
@tkittel tkittel changed the title Updates for building with NCrystal support Updates for building with NCrystal support (and fix CI) Feb 11, 2025
@tkittel
Copy link
Contributor Author

tkittel commented Feb 11, 2025

@pshriwise assuming the last non-NCrystal CI jobs also finish fine, I believe this PR is now ready. In summary:

  • It stops including a header file which was marked obsolete in NCrystal 4.0.0 (released yesterday).
  • It allows OpenMC CMake code to find NCrystal, even when NCrystal was installed via pip install ncrystal.
  • It fixes the current CI failures, by removing usage of features like ncrystal-config --setup which were obsoleted in the NCrystal 4.0.0. Instead it simply uses a pip installed ncrystal, where no special post-installation steps are needed. As a bonus, this avoids manual compilation of NCrystal freeing up a little bit of CI time.
  • It also updates a CI reference output file to be correct for NCrystal 4 (there were very minor wiggles in some output - most likely due to some tiny bug fixes in the new release (altering inelastic cross sections by 1e-4).

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks @tkittel!

@paulromano paulromano enabled auto-merge (squash) February 11, 2025 13:48
@paulromano paulromano merged commit 27ce2ce into openmc-dev:develop Feb 11, 2025
15 checks passed
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