845050 - FileCopier support for symlinks

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

The FileCopier code in mozpack is pretty useful! We're looking at using it as part of the build system for tasks like installing IDLs and header files into the dist directory.

It would be handy if FileCopier or some variant (possibly a new BaseFile class) supported symlinks (where available) instead of raw file copy. This should lead to some performance wins due to reduced I/O as part of building.

I don't think this is required for any build system work just yet. It will be nice to plug in if and when it is available.

Comment 1

[(not currently active) Ted Mielczarek]

We could presumably refactor the code out of nsinstall.py to do this. (I guess we could just make nsinstall.py use mozpack.)

Comment 2

[Gregory Szorc [:gps]]

Attachment: FileCopier support for symlinks

Here's my initial attempt. The implementation achieves correctness at
the expense of performance.

I'll request review once I've tested this a bit more (likely by hooking
up some kind of file install manifest to the build backend).

https://tbpl.mozilla.org/?tree=Try&rev=d00bd2bfb587

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: WIP on install_resources.

Just want to throw this out there.  I need this for Android resource
improvements.  There's some argparsing stuff to allow specifying
different roots for different manifests.  It's ugly.  Like:

	$(PYTHON) $(topsrcdir)/mobile/android/install_resources.py \
    $(DEFINES) \
    -v \
    --destdir=res \
    --root=$(srcdir)/resources \
    --manifest=$(srcdir)/resource-manifest.in \
    --root=$(srcdir)/resources \
    --manifest=$(srcdir)/android-services-resource-manifest.in \
    --root=$(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/res \
    --manifest=$(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/branding-resource-manifest.in

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 770870 [details] [diff] [review]
WIP on install_resources.

Review of attachment 770870 [details] [diff] [review]:
-----------------------------------------------------------------

I was essentially starting to build a manifest container that pretty much does what you are doing. Although, you may have slightly different requirements for your on-disk format, so perhaps it warrants separate code.

We don't yet have a separate bug to track the new manifest work. Perhaps we should morph this into that?

Comment 5

[Gregory Szorc [:gps]]

Attachment: FileCopier support for symlinks

This patch adds a SymlinkFile to mozpack. On the last push to try, all
platforms were happy except Windows, where the test for replacing an
existing file with a symlink was failing because skip_if_older was True.
I verified the trivial fix on a local Windows build.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 775808 [details] [diff] [review]
FileCopier support for symlinks

Review of attachment 775808 [details] [diff] [review]:
-----------------------------------------------------------------

One thing that bothers me is that there two types of symbolic links that we may want to support eventually:
- Symlinks to the original file, like here, that avoids a file copy
- Symlinks within the target directory

I think it would make sense to keep the SymlinkFile name for the latter.

I'm thinking there's maybe no reason to make that a separate class, and that it could just be an optional mode for File.copy.

::: python/mozbuild/mozpack/files.py
@@ +177,5 @@
> +        # falling back to file copy.
> +        if not hasattr(os, 'symlink'):
> +            return File.copy(self, dest, skip_if_older=skip_if_older)
> +
> +        # Always verify the path symlinking to exists.

# Always verify the symlink target path exists.

@@ +179,5 @@
> +            return File.copy(self, dest, skip_if_older=skip_if_older)
> +
> +        # Always verify the path symlinking to exists.
> +        if not os.path.exists(self.path):
> +            raise ErrorMessage('Path symlinking to does not exist: %s' % self.path)

'Symlink target path does not exist'

Comment 7

[Gregory Szorc [:gps]]

Attachment: FileCopier support for symlinks

I fixed the strings and renamed SymlinkFile to AbsoluteSymlinkFile and
added an os.path.isabs() check to ensure the symlink target is an
absolute path. I /think/ this addresses the review feedback.

https://tbpl.mozilla.org/?tree=Try&rev=120e74d96e62

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27857f7ab7d7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27857f7ab7d7