If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

FileCopier support for symlinks

RESOLVED FIXED in mozilla25

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
We could presumably refactor the code out of nsinstall.py to do this. (I guess we could just make nsinstall.py use mozpack.)
(Assignee)

Comment 2

4 years ago
Created attachment 770643 [details] [diff] [review]
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
(Assignee)

Updated

4 years ago
Assignee: nobody → gps
Created attachment 770870 [details] [diff] [review]
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
Attachment #770870 - Flags: feedback?(gps)
(Assignee)

Comment 4

4 years ago
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?
Attachment #770870 - Flags: feedback?(gps) → feedback+
(Assignee)

Updated

4 years ago
Blocks: 890097
(Assignee)

Comment 5

4 years ago
Created attachment 775808 [details] [diff] [review]
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.
Attachment #775808 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #770643 - Attachment is obsolete: true
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'
Attachment #775808 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 776803 [details] [diff] [review]
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
Attachment #776803 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #775808 - Attachment is obsolete: true
Attachment #776803 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/27857f7ab7d7
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/27857f7ab7d7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.