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

Tiny updates from experience building on Mac #2894

Merged
merged 13 commits into from
Apr 20, 2024

Conversation

gridley
Copy link
Contributor

@gridley gridley commented Mar 6, 2024

Description

This does a few things to tidy up and improve documentation:

  1. Remove all mentions of F90.
  2. Do not search for a C language compiler in CMake, since we don't use that.
  3. Use modern OpenMP linking in CMakeLists since we require 3.10 or above, which supports this.
  4. Add a quick install guide for Mac OS X, tested to work on my new computer.

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)

@gridley
Copy link
Contributor Author

gridley commented Mar 6, 2024

This addition to the quick install guide will hopefully prevent confusion as encountered in:

https://openmc.discourse.group/t/openmp-on-mac-fails-success/2070

@gridley gridley requested a review from pshriwise as a code owner March 7, 2024 16:54
@gridley
Copy link
Contributor Author

gridley commented Mar 7, 2024

It appears that the OpenMP target linking approach may have screwed with the dlopen source tests, which require cmake 3.3. Now 3.9 is required for anything linking to OpenMC, so I think that'd be the issue with those tests failing. I've bumped them up to match the main CMakeLists.txt version requirement. 3.10 came out in 2017, so it's definitely reasonable to require it everywhere.

@gridley
Copy link
Contributor Author

gridley commented Mar 9, 2024

Hm, this is the error I'm hitting in CI. Somehow, the OpenMP linking technique suggested by an old comment in the CMakeLists.txt file implemented in this PR breaks dynamically linked particle source compilation. I'm still not sure how to fix this, at the moment.

---------------------------- Captured stderr setup -----------------------------
CMake Error at /usr/local/lib/cmake/OpenMC/OpenMCTargets.cmake:68 (set_target_properties):
  The link interface of target "OpenMC::libopenmc" contains:

    OpenMP::OpenMP_CXX

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /usr/local/lib/cmake/OpenMC/OpenMCConfig.cmake:27 (include)
  CMakeLists.txt:5 (find_package)


CMake Warning:
  Manually-specified variables were not used by the project:

    OPENMC_USE_MPI


CMake Generate step failed.  Build files cannot be regenerated correctly.
_____________________ ERROR at setup of test_dlopen_source _____________________

I guess we need find_package for OpenMP in the CMakeLists for dynamically linked sources?

@gridley
Copy link
Contributor Author

gridley commented Apr 12, 2024

The solution was to find the OpenMP package in the cmake config file. This is now good for review if anyone can get to that, maybe @paulromano, @pshriwise, or @jtramm.

@paulromano
Copy link
Contributor

Thanks @gridley! I'll take a look shortly.

@jtramm
Copy link
Contributor

jtramm commented Apr 12, 2024

Thanks for putting these changes together! One thing I'm thinking is that I'm not sure that we should be saying that homebrew is strictly required. Definitely fine to say this is one way of doing it, but it's also pretty straightforward just to download a LLVM compiler binary (e.g., from here) and go from there. Mac ports is also very popular, so don't want people to think they need to change package managers just to install OpenMC if they are already using mac ports. Also wondering if it is a good idea to recommend installing xtensor or other submodules via homebrew, as then the version may be a mismatch between what OpenMC is expecting from its regular git submodule?

@gridley
Copy link
Contributor Author

gridley commented Apr 12, 2024

I have never seen a version of xtensor that doesn't work with openmc. We don't use a huge amount of features out of it. For the very small number of arch linux users that build through the AUR, there have not been reported issues taking the same approach.

I agree with you on that one John regarding homebrew! I'll add a note that this is just one option.

@gridley
Copy link
Contributor Author

gridley commented Apr 12, 2024

Alright, it now says that homebrew is not the only option. I think the point of the quick install guide is just to give a quick and easy way to do stuff, so specifically mentioning the other possibilities doesn't make sense to me. Especially because I've not tested them. As for downloading a compiler binary, that still leaves the user to go and download binaries for the other libraries we depend on, which sounds like a real pain to get it to all work together, potentially.

@gridley
Copy link
Contributor Author

gridley commented Apr 12, 2024

Why might this happen in CI?

  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for mpi4py
Failed to build mpi4py

It was passing and all I did was change a docs file. hm!

@paulromano
Copy link
Contributor

@gridley See my comment here: #2958 (comment)

@gridley
Copy link
Contributor Author

gridley commented Apr 12, 2024

Thank you for your prompt response Paul!

@gridley
Copy link
Contributor Author

gridley commented Apr 13, 2024

This PR now depends on #2958 going through first.

@gridley
Copy link
Contributor Author

gridley commented Apr 14, 2024

Hooray, tests are passed! Thanks Paul!

@shimwell
Copy link
Member

Slightly related to this PR as I think I've come across the same issues, I have started adapting the CI so that it does some CI on Mac OS

The CI which is based on openmc/develop branch currently gives No CMAKE_Fortran_COMPILER could be found. when trying to build on mac-latest.

I look forward to trying the CI again if this PR gets merged as it looks like it solves the error I'm seeing

@gridley
Copy link
Contributor Author

gridley commented Apr 17, 2024

As an update, I have heard from one other person that this branch fixed a CMake OpenMP linking failure when building on Mac.

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 for these updates @gridley!

@paulromano paulromano enabled auto-merge (squash) April 20, 2024 03:43
@paulromano paulromano merged commit b53b601 into openmc-dev:develop Apr 20, 2024
18 checks passed
@shimwell
Copy link
Member

I've updated that branch with the Mac OS CI runner with these latest changes, unrelated to openmc but it looks like NJOY wants fortran.

+ cd NJOY2016
+ mkdir build
+ cd build
+ cmake -Dstatic=on ..
-- Found Python3: /Users/runner/hostedtoolcache/Python/3.10.14/x64/bin/python3.10 (found suitable version "3.10.14", minimum required is "3.5") found components: Interpreter
-- The Fortran compiler identification is unknown
CMake Error at CMakeLists.txt:8 (project):
  No CMAKE_Fortran_COMPILER could be found.
  Tell CMake where to find the compiler by setting either the environment
  variable "FC" or the CMake cache entry CMAKE_Fortran_COMPILER to the full
  path to the compiler, or to the compiler name if it is in the PATH.
-- Configuring incomplete, errors occurred!

I shall add fortran to the export path once I figure out where it is installed to

https://github.com/shimwell/openmc/actions/runs/8767146139/job/24060073439

@gridley gridley deleted the build-updates branch July 10, 2024 18:22
church89 pushed a commit to openmsr/openmc that referenced this pull request Jul 18, 2024
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