Skip to content

Conversation

@RReichert
Copy link
Contributor

@RReichert RReichert commented Sep 10, 2020

Background

There were two issues during the restructuring of our code base.

The first change that needed to be done was fix the create_source_search_paths function. After adding the starling folder in the root directory, FindStarling.cmake script's usage of SOURCE_DIR meant that the ${CMAKE_CURRENT_SOURCE_DIR}/${x_SOURCE_DIR} folder was choose as the folder for which it would call add_subdirectory on to introduce the starling repo's cmake targets.

The second issue was simply that some starling libraries weren't being flagged as exportable by the subdirectory, which meant that if we were to use -DTHIRD_PARTY_INCLUDES_AS_SYSTEM=false cmake cache variable, it would fail static analysis as clang-tidy would complain about eigen values.

@RReichert RReichert force-pushed the rodrigor/starling-third-party-fix branch from f861c13 to fc47377 Compare September 13, 2020 20:55
@RReichert RReichert requested a review from woodfell September 16, 2020 05:11
Copy link
Contributor

@woodfell woodfell left a comment

Choose a reason for hiding this comment

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

One request to fix faulty search paths, other than that looks good

else()
list(APPEND x_SOURCE_SEARCH_PATHS "${CMAKE_CURRENT_SOURCE_DIR}/${x_SOURCE_DIR}")
list(APPEND x_SOURCE_SEARCH_PATHS "${CMAKE_CURRENT_SOURCE_DIR}/third_party/${x_SOURCE_DIR}")
list(APPEND x_SOURCE_SEARCH_PATHS "${PROJECT_SOURCE_DIR}/${x_SOURCE_DIR}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the real issue here is that this line shouldn't be here, neither should the one 2 lines above. A long time ago we agreed that all submodules will live under third_party, these modules shouldn't be searching any part that doesn't have third_party in it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a good idea to remove the old system. I've since rolled back the changes and used the suggested approach.

@RReichert RReichert force-pushed the rodrigor/starling-third-party-fix branch from 645b78f to c9225eb Compare September 28, 2020 00:30
GenericFindDependency(
TARGET protobuf-nanopb
SOURCE_DIR "third_party/nanopb"
SOURCE_DIR "nanopb"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was needed to be updated since one of the deleted paths was working with the original third_party/nanopd.

@RReichert RReichert requested a review from woodfell September 28, 2020 02:20
@RReichert
Copy link
Contributor Author

NOTE these changes are currently being used by the https://github.com/swift-nav/swiftlets/pull/880 PR.

Copy link
Contributor

@woodfell woodfell left a comment

Choose a reason for hiding this comment

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

LGTM!

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