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

Makefile: streamline cmake-build & cleanup nuttx_, _default targets #11533

Merged
merged 3 commits into from
Feb 26, 2019

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Feb 24, 2019

Test data / coverage
Cygwin Windows SITL, draft unit test builds and some manual artificial test target builds to check for corner cases.

Describe problem solved by the proposed pull request

  1. CMake invocations using the makefile function cmake-build were limited to the -DCONFIG=$(PX4_CONFIG) cmake parameter. There are currently multiple other makefile targets e.g. python_coverage, scan-build, px4_sitl_default-clang, ... that also run cmake with their own invocation which could easily be streamlined to just calling the cmake-build function that containes the latest tweaks for generator transitions, space escaping, caching. I came across this solution since I'm looking into how to best unify and improve unit testing setup (see the early draft here: master...MaEtUgR:gtest#diff-b67911656ef5d18c4ae36cb6741b7965R323).
  2. Switched to the cmake --build ... command to abstract the actually used build tool (see https://cmake.org/cmake/help/v2.8.11/cmake.html#opt%3a--builddir).
  3. The NUTTX_CONFIG_TARGETS target list was removed by @dagar in f692ad0#diff-b67911656ef5d18c4ae36cb6741b7965L162 because the nuttx_ prefix does not exist anymore. If this filtered multi target is still needed the list generation needs to be reimplemented anyways.
  4. If we already have a multitarget for all defaults I also added one for all the targets ALL_CONFIG_TARGETS for testing purposes
  5. Improved cmake reconfiguration necessity detection see Makefile: streamline cmake-build & cleanup nuttx_, _default targets #11533 (comment)
  6. Readability, comments

@MaEtUgR MaEtUgR self-assigned this Feb 24, 2019
@MaEtUgR MaEtUgR requested a review from dagar February 24, 2019 13:23
@MaEtUgR MaEtUgR changed the title Makefile: streamline cmake-build & cleanup nuttx_, _default targets Makefile: streamline cmake-build & cleanup nuttx_, _default targets Feb 24, 2019
@MaEtUgR MaEtUgR force-pushed the makefile-cmake-refactor branch 2 times, most recently from 00a04e5 to b5d82d6 Compare February 24, 2019 14:08
@dagar
Copy link
Member

dagar commented Feb 24, 2019

  1. CMake invocations using the makefile function cmake-build were limited to the -DCONFIG=$(PX4_CONFIG) cmake parameter. There are currently multiple other makefile targets e.g. python_coverage, scan-build, px4_sitl_default-clang, ... that also run cmake with their own invocation which could easily be streamlined to just calling the cmake-build function that containes the latest tweaks for generator transitions, space escaping, caching. I came across this solution since I'm looking into how to best unify and improve unit testing setup (see the early draft here: master...MaEtUgR:gtestdiff-b67911656ef5d18c4ae36cb6741b7965R323).

Sounds good as long as we can maintain control of the options as needed.

  1. Switched to the cmake --build ... command to abstract the actually used build tool (see https://cmake.org/cmake/help/v2.8.11/cmake.html#opt%3a--builddir).

I need to check the current state, but at one point this was intentional. There were issues with things like quashing console color output.

  1. The CONFIG_TARGETS_DEFAULT target list can be simplified to not remove the default_ prefix, this was probably necessary in the past but only makes things more complex without benefit now.

You mean the optional _default postfix? Eg px4_fmu-v5 == px4_fmu-v5

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 24, 2019

Thanks for your immediate response!

Sounds good as long as we can maintain control of the options as needed.

We should not go crazy on cmake options if it's avoidable but there are certain cases where configure time decision is necessary to achieve a certain customization.

I need to check the current state, but at one point this was intentional. There were issues with things like quashing console color output.

If you know any such specific case I would like to test that to make sure there are no disadvantages. I was expecting cmake --build to take care of more corner cases than handling the distinction manually.

You mean the optional _default postfix? Eg px4_fmu-v5 == px4_fmu-v5

You mean px4_fmu-v5 == px4_fmu-v5_default. Yes and I made a fallacy there, let me fix that quickly, forget that point.

@dagar
Copy link
Member

dagar commented Feb 24, 2019

I need to check the current state, but at one point this was intentional. There were issues with things like quashing console color output.

If you know any such specific case I would like to test that to make sure there are no disadvantages. I was expecting cmake --build to take care of more corner cases than handling the distinction manually.

I gave it a try with current cmake and everything seems fine so far. Let's go for it.

@MaEtUgR MaEtUgR changed the title Makefile: streamline cmake-build & cleanup nuttx_, _default targets [WIP] Makefile: streamline cmake-build & cleanup nuttx_, _default targets Feb 24, 2019
@MaEtUgR MaEtUgR force-pushed the makefile-cmake-refactor branch from 8ef37e4 to 89f2cbf Compare February 24, 2019 18:50
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 24, 2019

I restored the "you can omit _default" feature again in the last force push. I didn't find a way to wildcard the phony targets such that the abbreviated targets directly depend on the normal targets and there's no duplication.

by comparing the configuration options that cmake reports
from the cache with the ones from the current build
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 25, 2019

@dagar I found a way to check if rerunning cmake is necessary because the configuration options are now different than the ones in the cache of the build directory 67eb0ea I can achieve the behavior of minimal reconfiguration recompilation I described to you with this.

default:

  • when running sitl it configures and compiles
  • when rerunning sitl it only reconfigures on cmakefile changes and recompiles on source changes

new:

  • when adding an additional configuration like enable unit testing or coverage checks and so on but still with sitl it reconfigures because changes in the configuration are detected but only recompiles parts that are really necessary with the new configuration
  • when rerunning the same additional configuration it doesn't need to reconfigure which e.g. drastically speeds up small unit test changes
  • when additional options are not disabled but only omitted next time to run sitl no need to reconfigure or recompile

This mechanism could maybe even be used to compile e.g. different px4_sitl_XXX target configurations in the same folder such that not everything has to be recompiled from scratch to an almost identical copy.

@MaEtUgR MaEtUgR changed the title [WIP] Makefile: streamline cmake-build & cleanup nuttx_, _default targets Makefile: streamline cmake-build & cleanup nuttx_, _default targets Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants