Page MenuHomePhabricator

Use ninja to generate dep files for the native build

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



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 # Check native build isn't ran again.
ninja # Native build runs again.

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Mar 31 2020, 19:23
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 31 2020, 19:23
deadalnix updated this revision to Diff 17334.Mar 31 2020, 20:19

remove leftover comment

deadalnix updated this revision to Diff 17335.Mar 31 2020, 20:42

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.

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
deadalnix added inline comments.Apr 1 2020, 12:36
94 ↗(On Diff #17335)

This is python's style.

Fabien added inline comments.Apr 1 2020, 12:40
94 ↗(On Diff #17335)

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

deadalnix added inline comments.Apr 1 2020, 14:18
77 ↗(On Diff #17335)


deadalnix updated this revision to Diff 17354.Apr 1 2020, 14:18

Address comments

Fabien accepted this revision.Apr 1 2020, 14:38
Fabien added inline comments.
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
deadalnix added inline comments.Apr 1 2020, 14:56
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.

This revision was automatically updated to reflect the committed changes.