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

#362: Remove repeated searches for Boost that were picking up mixed versions #564

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Jul 10, 2023

Having multiple calls to find_package(Boost), some specifying a required version and others not, led to CMake searching for the package multiple times, and flipping imports between found instances depending on which one was found last (or maybe within the calling scope). This could result in a quietly and inscrutably broken build, with ABI mismatches and ODR violations.

Address these concerns by removing all of the find_package calls other than the one with the specified version at the root of the package.

Fixes #362

Additions

  • N/A

Removals

  • N/A

Changes

  • Drop find_package(Boost) calls from subsidiary packages' CMakeLists.txt

Testing

Screenshots

See #362

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

@mattw-nws
Copy link
Contributor

Ugh! While, technically, I don't think we could get into an ABI mismatch situation if we were only using header-resident components (right?) yeah I can see how this could lead to major headaches while doing iterative builds during development if there were mismatches in API versions picked up over time. @hellkite500, is there any reason to keep the find_boost calls in the subdirectory CMakeLists.txt files? I don't think there was really ever an intent for those "libraries" to be able to be built independently...?

@PhilMiller
Copy link
Contributor Author

If any types in those headers differed between versions and were passed across function calls, then you'd have an ABI mismatch.

However, even if there were just distinct callers to function templates that came from different versions, you'd still have violations of the One Definition Rule. Having actually debugged a failure resulting from an ODR violation, it's not fun

@PhilMiller
Copy link
Contributor Author

Also, if you look at my notes on #362, I was getting version mismatches in a single, clean build

@mattw-nws mattw-nws added bug Something isn't working build Issues related to CMake and building ngen labels Jul 10, 2023
@mattw-nws mattw-nws merged commit 060d735 into NOAA-OWP:master Jul 12, 2023
20 checks passed
@PhilMiller PhilMiller deleted the 362-boost-import branch July 28, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build Issues related to CMake and building ngen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting ${BOOST_ROOT} doesn't seem to help -- Boost has to be system searchable
3 participants