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

Fix linker errors when building with shared libs on macOS Apple Silicon #6036

Merged

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

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
22 checks passed
@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