Skip to content

Conversation

@geoffrey4444
Copy link
Contributor

Proposed changes

I have succeeded in building spectre, with all tests passing, on macOS using shared libraries. (Previously, we required macOS Apple Silicon users to build with static libraries.) But to make this work, I had to fix some linker errors that appeared:

  • I added an explicit instantiation for detail::initialize_coords_and_jacobians() in src/Elliptic/DiscontinuousGalerkin/Initialization.cpp. Otherwise, I get an undefined symbol linker error when trying to build this file.
  • I added includes for PartialDerivatives.hpp and PartialDerivatives.tpp to a bunch of our executables, since when building them I was getting linker errors about partial_derivatives being undefined symbols.

A followup PR will update the macOS apple silicon docs.

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@geoffrey4444 geoffrey4444 requested a review from nilsvu May 29, 2024 15:52
@geoffrey4444 geoffrey4444 mentioned this pull request May 29, 2024
3 tasks
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

We shouldn't be including the tpp files in any hpps. I would expect there to be an explicit instantiation that handles this. Why is this not an issue on Linux systems? Or phrased differently, why is there no explicit instantiation for the specific case that you need already generated?

@geoffrey4444
Copy link
Contributor Author

We shouldn't be including the tpp files in any hpps. I would expect there to be an explicit instantiation that handles this. Why is this not an issue on Linux systems? Or phrased differently, why is there no explicit instantiation for the specific case that you need already generated?

So I understand, could you please explain the problem with including top files in hpp files?

I don't know why this only seems to break when I try building with shared libraries on Mac and not on Linux. This was just the first thing I learned how to do that let the shared-libraries-on-Apple-Silicon build succeed.

I'd be happy to change this to adding explicit instantiations, but I wasn't sure where to add them or how many different partial derivatives instantiations I might have to add. Do you have any suggestions?

@nilsdeppe
Copy link
Member

Ah, sorry! So the idea behind tpps is that you can include them in cpp files to do explicit instantiations. Then by only including hpp files in other places the compiler doesn't have to do implicit instantiations and this should reduce build time.

So I think it should be only 1 per system. Let's chat on the technical call Thursday if you can make it to see if we can find a way that would make it 1) easy to instantiate and 2) solve the issue and allow you to build shared libs :)

@geoffrey4444
Copy link
Contributor Author

Sounds good, Nils! I should be able to make the technical call, and a discussion to find a path forward for this PR would be great.

By the way, I didn't see this coming because many of our hpp's already include tpp's, including the following:

  • TimeDependence.hpp
  • DgOperator.hpp
  • ComputeTimeDerivative.hpp
  • EvolveGhBinaryBlackHole.hpp
  • EvolveGhSingleBlackHole.hpp
  • EvolveGhValenciaDivCleanWithHorizon.hpp
  • EvolveM1Grey.hpp
  • EvolveScalarTensorSingleBlackHole.hpp
  • ConservativeSystem.hpp

@geoffrey4444 geoffrey4444 force-pushed the FixMacOSSharedLinkerErrors branch from 0d69a6f to 8e22e65 Compare May 30, 2024 21:41
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!!

@geoffrey4444
Copy link
Contributor Author

You're welcome, @nilsdeppe ! @nilsvu do you still want to take a look at this PR? If so it's ready whenever you get a chance

@nilsvu nilsvu enabled auto-merge May 31, 2024 15:16
@nilsvu nilsvu merged commit 57fc561 into sxs-collaboration:develop May 31, 2024
@nilsvu nilsvu added the build system CMake build system label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build system CMake build system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants