Bug 1382739 added support to the "copier" interface, now it needs to be integrated into InstallManifest

Comment on attachment 8889992 [details] Bug 1384224 - Add support for hardlinks to InstallManifest; https://reviewboard.mozilla.org/r/161052/#review166376 A number of callers create InstallManifest instances. From their perspective, their focus is "install file X." They aren't particularly concerned with the mechanism used, as that can vary by platform support. Symlinks and copies are effectively expressing the intent of coupling between the existing and new file. If you want independence, copy. If not, symlink. The conversion of symlinks to copies on platforms that don't support symlinks is an implementation detail that should mostly be hidden. For hard links, I don't think InstallManifest should be expressing the need for a hard link versus a symlink or copy. Instead, it should be expressing an intent to create a new file from an existing file with an allowed set of properties. These intents then get mapped to copy, symlink, hard link, junction point, etc as part of converting the InstallManifest entries to a FileRegistry/Copier depending on run-time support or policy. Could you please spend a few minutes to consider how you would define file installs in a mechanism-agnostic way? Could "symlink" and "hard link" both be expressed as a generic "link" for example? Would you need a "link but not with a pointer" primitive? I just want to make sure the door is open to e.g. transitioning to links on Windows without having to go through and update all InstallManifest populators to use a different primitive that supports links. I'd rather we express intents for file behavior then map to supported primitives at run time. It's quite possible that at the end of this exercise you arrive at 3 primitives (copy, link via a pointer, and file alias) and that these are best expressed in the API as "copy," "symlink," and "hard link." After you've done that, I'll take a more thorough look at this patch. Also, if you are worried about scope bloat, I'll take over development of this feature if you no longer want to work on it: adding support for links on Windows is long-overdue. ::: python/mozbuild/mozpack/manifests.py:89 (Diff revision 1) > Version 3 added support for pattern entries. > Version 4 added preprocessed file support. > Version 5 added content support. > """ > > CURRENT_VERSION = 5 This needs to be bumped any time the manifest's features change so there isn't a mismatch between client version and persisted file version.

Comment on attachment 8889992 [details] Bug 1384224 - Add support for hardlinks to InstallManifest; https://reviewboard.mozilla.org/r/161052/#review166376 I'm reasonably certain that for the purposes of our builds, symlink and hardlink can basically always be used interchangeably. I think copy is sufficiently different that it should not be used interchangeably. Given that I'd propose the following: - Consolidate all hardlink/symlink in this patch to be "link" - `InstallManifest` grows a constructor parameter for `link_policy`; it can default to symlink for backwards compatibility - `populate_registry` handles links based on the current `link_policy` - `InstallManifestNoSymlinks` can become `InstallManifestNoLinks` for consistency Once your other patchset (bug 1382697) lands we can plumb the file install mode option through to the construction of `InstallManifest` where appropriate -- I think the only important one for building Firefox is `process_install_manifest.py`. WDYT?

Comment on attachment 8889992 [details] Bug 1384224 - Add support for hardlinks to InstallManifest; https://reviewboard.mozilla.org/r/161052/#review166376 I think differentiating between "copy" and "link" in InstallManifest is fine. We can preserve the add_symlink() method as an alias to add_link() if you want. But all consumers should be in-tree and I'd encourage you to do a bulk rename. But can be deferred to follow-up. I'm not convinced InstallManifest needs to be tied to a link policy. It is just a dumb data structure after all. `populate_registry` is the place where a link policy would come into play. I think we should formalize the type conversion in `populate_registry`. We can then remove the `InstallManifestNoSymlinks` hacks. Although we may still want a helper function in manifests.py that takes an `InstallManifest` and a `subst` or `ConfigEnvironment` and populates a registry.

Comment on attachment 8889992 [details] Bug 1384224 - Add support for hardlinks to InstallManifest; https://reviewboard.mozilla.org/r/161052/#review166376 > This needs to be bumped any time the manifest's features change so there isn't a mismatch between client version and persisted file version. I don't think this is required now -- the changes at this point are backwards compatible for persisted files.

Comment on attachment 8889992 [details] Bug 1384224 - Add support for hardlinks to InstallManifest; https://reviewboard.mozilla.org/r/161052/#review167094 Ahh, much better! I agree that we no longer need to bump the manifest version since the on-disk format is backwards compatible. I'm a bit hesitant to actually use hard links without first auditing copier.py for assumptions about "not a hard link." It /should/ be OK. But the code has historically been sensitive. There is certainly an opportunity to look at `st_nlink` and optimize away operations that don't need performed. But this can all be done in another bug.

Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a7f3328de88d Add support for hardlinks to InstallManifest; r=gps