Support symlinks in nsinstall.py

ASSIGNED
Assigned to

Status

Firefox Build System
General
ASSIGNED
7 years ago
5 months ago

People

(Reporter: gps, Assigned: sid0)

Tracking

Trunk

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
This is a follow-up from bug 680636 to have the Python version of nsinstall, nsinstall.py, have feature parity with the original C version with regards to symlinks.

Currently, nsinstall.py ignores symlinks options. It should be made to honor them.
(Reporter)

Comment 1

7 years ago
I think I fat fingered the bug number when I created this bug.
Blocks: 680636
No longer depends on: 680536
(Reporter)

Comment 2

7 years ago
Created attachment 590971 [details] [diff] [review]
Basic symlink support in nsinstall.py

This patch adds basic symlink support to nsinstall.py. If -l, -L, or -R are specified, it creates symlinks instead of copying files. The expected behavior of -R is ignored. I /think/ this is acceptable. If you think it is needed and can explain how it is supposed to work, I can code it up.

In nsinstall.c, the mtime of the symlink and its source are compared when determining whether to unlink the existing symlink. I did not port this. This /could/ have an impact on makefile freshness determination. But, I /think/ that would only come into play if the tester were using lstat() instead of stat(). I'm not sure what make/pymake uses internally. If you don't want to risk it, I could certainly port this logic forward.

I have this patch applied on top of one for bug 680636 and the tree appears to mostly build fine! Although, I did discover that nsinstall.py isn't performing thread-safe directory creation (os.mkdir will raise if the path exists and multiple nsinstall.py invocations could happen in parallel). I can make the necessary changes in this patch or deal with it elsewhere.

Finally, I don't like the Python style used in this patch. But, it matches what existed previously. To fix the style would require rewriting the entire file. I'm not about to make Ted review a completely new nsinstall.py ;)
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #590971 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 3

7 years ago
Created attachment 596146 [details] [diff] [review]
patch including Windows support for junctioning dirs (WIP)

I've added a "win32" Python module dir and moved all the win32 junk we had in JarMaker.py into modules in there.
Assignee: gps → sagarwal
Attachment #590971 - Attachment is obsolete: true
Attachment #590971 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 4

7 years ago
Ugh, this is never going to work on Windows. rm -rf follows junctions :(
(Assignee)

Comment 5

7 years ago
Created attachment 596207 [details] [diff] [review]
updated patch including Windows support (for posterity, shouldn't be used)
Attachment #596146 - Attachment is obsolete: true
Note, should this come up again -- doing this with junctions isn't the right way.  NTFS has real symbolic links since Vista; see http://msdn.microsoft.com/en-us/library/windows/desktop/aa365682%28v=vs.85%29.aspx for details.
(Reporter)

Comment 7

5 years ago
In my ideal world nsinstall and its Python variant are obsoleted in favor of the installation code in mozpack (python/mozbuild/mozpack). The code in the latter is more intelligent about copying only when things change, etc.
Copying doesn't help; we need actual symlinks for developers so that dependencies on header files work properly.
(Reporter)

Comment 9

5 years ago
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #8)
> Copying doesn't help; we need actual symlinks for developers so that
> dependencies on header files work properly.

That's why we are switching to using manifests for managing header installs (bug 896797). Up until bug 884587, we completely blew away dist/include at the top of builds, forcing file re-copy. We now maintain manifests of which files actually belong so as to not incur unnecessary copying. The next step is to actually install these files with install manifests instead of nsinstall.

Comment 10

5 years ago
(In reply to comment #9)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #8)
> > Copying doesn't help; we need actual symlinks for developers so that
> > dependencies on header files work properly.
> 
> That's why we are switching to using manifests for managing header installs
> (bug 896797). Up until bug 884587, we completely blew away dist/include at the
> top of builds, forcing file re-copy. We now maintain manifests of which files
> actually belong so as to not incur unnecessary copying. The next step is to
> actually install these files with install manifests instead of nsinstall.

The point is that we want to avoid the copy stage necessary to make the change made to a header in your srcdir be visible in the version in objdir/dist/include.

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.