1390916 - Process install manifests with --track in the recursive make backend

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Michael Shal [:mshal]]

While looking into https://bugzilla.mozilla.org/show_bug.cgi?id=902825#c48, I needed to use the newer --track style of install manifests instead of the older --no-remove style. This should be useful regardless of the outcome of bug 902825 since it provides finer grained tracking of installed files.

For example, removing [test_client.js] from services/fxaccounts/tests/xpcshell/xpcshell.ini and running the install-test-files target will not correctly remove test_client.js from the objdir/_tests/ directory unless we use --track.

Comment 1

[Michael Shal [:mshal]]

Attachment: Bug 1390916 - Make .PHONY rule match the actual make targets;

Review commit: https://reviewboard.mozilla.org/r/169686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169686/

Comment 2

[Michael Shal [:mshal]]

Attachment: Bug 1390916 - Remove references to dist/sdk install manifest;

The SDK installation code was removed in bug 1333826.

Review commit: https://reviewboard.mozilla.org/r/169688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169688/

Comment 3

[Michael Shal [:mshal]]

Attachment: Bug 1390916 - Always use --track instead of --no-remove for install manifests;

The --track flag provides a more accurate accounting of what files were
installed by the manifest, so they can be appropriately removed. For
example, test files are now removed from _tests if an entry in a test
file is deleted.

The --no-remove flag is removed as an alternative, and the --track flag
is now mandatory.

Review commit: https://reviewboard.mozilla.org/r/169690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169690/

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8898336 [details]
Bug 1390916 - Make .PHONY rule match the actual make targets;

https://reviewboard.mozilla.org/r/169686/#review175360

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8898337 [details]
Bug 1390916 - Remove references to dist/sdk install manifest;

https://reviewboard.mozilla.org/r/169688/#review175362

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8898338 [details]
Bug 1390916 - Always use --track instead of --no-remove for install manifests;

https://reviewboard.mozilla.org/r/169690/#review175372

Please ask for re-review once you've replied to my question on changing the default behavior for directory symlinks and empty directories.

::: python/mozbuild/mozbuild/action/process_install_manifest.py:63
(Diff revision 1)
> -        remove_all_directory_symlinks=remove_all_directory_symlinks,
> -        remove_empty_directories=remove_empty_directories)
> +        remove_all_directory_symlinks=False,
> +        remove_empty_directories=False)

The default behavior is changed here. Is this intentional? If so, could you please rationalize why the new behavior is proper?

Comment 7

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #6)
> ::: python/mozbuild/mozbuild/action/process_install_manifest.py:63
> (Diff revision 1)
> > -        remove_all_directory_symlinks=remove_all_directory_symlinks,
> > -        remove_empty_directories=remove_empty_directories)
> > +        remove_all_directory_symlinks=False,
> > +        remove_empty_directories=False)
> 
> The default behavior is changed here. Is this intentional? If so, could you
> please rationalize why the new behavior is proper?

Good catch - I think I misinterpreted how these flags were used by copier.py in the case where we have an existing track file. It looks like we do need them to be True in that case in order to correctly remove a directory when files are no longer installed there. I'll update the patch...

Comment 8

[Michael Shal [:mshal]]

Comment on attachment 8898336 [details]
Bug 1390916 - Make .PHONY rule match the actual make targets;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169686/diff/1-2/

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8898337 [details]
Bug 1390916 - Remove references to dist/sdk install manifest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169688/diff/1-2/

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8898338 [details]
Bug 1390916 - Always use --track instead of --no-remove for install manifests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169690/diff/1-2/

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8898338 [details]
Bug 1390916 - Always use --track instead of --no-remove for install manifests;

https://reviewboard.mozilla.org/r/169690/#review176054

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c9a5812fc16
Make .PHONY rule match the actual make targets; r=gps
https://hg.mozilla.org/integration/autoland/rev/1b502238070f
Remove references to dist/sdk install manifest; r=gps
https://hg.mozilla.org/integration/autoland/rev/3a181cd6052b
Always use --track instead of --no-remove for install manifests; r=gps

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7c9a5812fc16
https://hg.mozilla.org/mozilla-central/rev/1b502238070f
https://hg.mozilla.org/mozilla-central/rev/3a181cd6052b