Page MenuHomePhabricator

Use ninja to generate dep files for the native build
ClosedPublic

Authored by deadalnix on Mar 31 2020, 19:23.

Details

Summary

This introduce a python script that is able to generate dep files from ninja.
The script is used to generate a proper dep file for re-running cmake for the native build, but it is generic and can be used for the native targets in the future.

Depends on D5624

Test Plan
ninja
ninja # Check native build isn't ran again.
touch build.ninja
ninja # Native build runs again.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fullnativedeps
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10063
Build 17960: Default Diff Build & Tests
Build 17959: arc lint + arc unit

Event Timeline

Use something simpler for extra dependencies.

Fabien requested changes to this revision.Apr 1 2020, 08:51
Fabien added a subscriber: Fabien.

If you have a document for the depfile format, please link it somewhere.

A few nits, but overall LGTM. I tested with various cmake and ninja versions, as well as OSX.

cmake/utils/gen-ninja-deps.py
39 ↗(On Diff #17335)

Use command -v instead, which is not portable.

77 ↗(On Diff #17335)

Are they guaranteed to be sorted that way ? If not you want to continue on order dependency instead of breaking

94 ↗(On Diff #17335)

Remove the blank lines

136 ↗(On Diff #17335)

Move to rebase_deps

This revision now requires changes to proceed.Apr 1 2020, 08:51
cmake/utils/gen-ninja-deps.py
94 ↗(On Diff #17335)

This is python's style.

cmake/utils/gen-ninja-deps.py
94 ↗(On Diff #17335)

The consider using docstring instead. Having a floating comment like this makes no sense.

cmake/utils/gen-ninja-deps.py
77 ↗(On Diff #17335)

yes

Fabien added inline comments.
cmake/utils/gen-ninja-deps.py
135 ↗(On Diff #17354)

This one is still there, but should be local to rebase_deps since it's the only place it is used.

This revision is now accepted and ready to land.Apr 1 2020, 14:38
cmake/utils/gen-ninja-deps.py
135 ↗(On Diff #17354)

It's a command line parameter, so it's obviously not scoped to that function. The fact that it happens to be used only there is simply an implementation detail.