Skip to content

Conversation

@woodfell
Copy link
Contributor

@woodfell woodfell commented Apr 29, 2019

This PR implements some common cmake functions used for the dependency resolution and provides standard Find* modules for common packages.

The two main new modules are:

GenericFindDependency
A function to search for a dependency. Searches in either bundled source code or system libraries. Configurable through either function parameters or global variables defined on the command line or elsewhere at configure time. Don't call directly, call from a Find* style cmake module.

SwiftCmakeOptions
Sets up and configures several generic options to enable or disable package features such as unit tests, documentation, or example code.

GenericFindDependency.cmake and SwiftCmakeOptions.cmake contain header with documentation of parameters and usage. The PR also includes several Find* modules for commonly used packages including all swift repositories. See other repos for examples of SwiftCmakeOptions.

As part of this work the new modules are being imported to other swift repositories.
Related PRs:

libsbp - swift-nav/libsbp#692
librtcm - https://github.com/swift-nav/librtcm/pull/85
libswiftnav - https://github.com/swift-nav/libswiftnav/pull/85
gnss-converters https://github.com/swift-nav/gnss-converters/pull/193
libsettings - swift-nav/libsettings#37
starling - https://github.com/swift-nav/starling/pull/2273
albatross - swift-nav/albatross#126
orion_proto - https://github.com/swift-nav/orion_proto/pull/31
orion-engine - https://github.com/swift-nav/orion-engine/pull/120
orion - https://github.com/swift-nav/orion/pull/400

@woodfell woodfell force-pushed the woodfell/create_standard_find_modules branch from b9210fb to ecc36bd Compare May 1, 2019 03:44
@woodfell woodfell force-pushed the woodfell/create_standard_find_modules branch from 207a416 to d669f1e Compare May 9, 2019 05:49
@woodfell woodfell force-pushed the woodfell/create_standard_find_modules branch from 33a6151 to 1fcb326 Compare May 9, 2019 06:37
@woodfell woodfell force-pushed the woodfell/create_standard_find_modules branch from f12cfd8 to ab73780 Compare May 15, 2019 05:30
@woodfell
Copy link
Contributor Author

@benjaminaltieri @jbangelo @martin-swift
I've finished off the new clang tools modules. I have split them up in to ClangTidy and ClangFormat since not every project uses both. Documentation at the header should be fairly self explanatory.

I have updated all the other PRs to use these modules and removed all the temporary hack targets. The only project which doesn't use the generic tidy/format targets is starling. It still uses this module but it has custom scripts in the repo. Starling is probably worth revisiting though, the script uses some hard coded flags to clang-tidy which passes, but as soon as it is run against the compile command database exported from cmake it throws loads of errors.

@woodfell woodfell requested review from pmiettinen and silverjam May 29, 2019 09:42
@pmiettinen
Copy link

Should default SN .clang-tidy file be provided? See for example https://github.com/swift-nav/starling/blob/master/.clang-tidy

@woodfell
Copy link
Contributor Author

Should default SN .clang-tidy file be provided? See for example https://github.com/swift-nav/starling/blob/master/.clang-tidy

@jbangelo @martin-swift
I don't have any strong opinions on this, but I can look in to it. Does anyone else have any thoughts?

@benjaminaltieri
Copy link
Contributor

@jbangelo @martin-swift
I don't have any strong opinions on this, but I can look in to it. Does anyone else have any thoughts?

let's let individual repos decide what linting they use. it's important that the module supports pulling this in (possibly a custom script as well? i.e. not only .clang_tidy file)

Copy link
Contributor

@benjaminaltieri benjaminaltieri left a comment

Choose a reason for hiding this comment

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

IGTM, would like a couple small tweaks if you approve or happy to hear other ideas

@benjaminaltieri benjaminaltieri dismissed their stale review May 31, 2019 01:20

items were fixed

Copy link
Contributor

@benjaminaltieri benjaminaltieri left a comment

Choose a reason for hiding this comment

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

looks good, for any CI that fails to create the clang targets we can just set ENABLE_CLANG_TIDY and ENABLE_CLANG_FORMAT to OFF in the travis.yml or travis.sh I'll revise that idea, I'm fine to go back to the previous behavior of not defining a clang target if we can't find the tool

@woodfell woodfell force-pushed the woodfell/create_standard_find_modules branch from 3dd6a7c to 3c390e5 Compare May 31, 2019 07:54
@benjaminaltieri benjaminaltieri removed the request for review from pmiettinen June 3, 2019 19:50
@woodfell woodfell merged commit ed44946 into master Jun 6, 2019
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.

7 participants