680968 - Support symlinks in nsinstall.py

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

I think I fat fingered the bug number when I created this bug.

Comment 2

[Gregory Szorc [:gps]]

Attachment: 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 ;)

Comment 3

[Rain [:rain1] (inactive)]

Attachment: 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.

Comment 4

[Rain [:rain1] (inactive)]

Ugh, this is never going to work on Windows. rm -rf follows junctions :(

Comment 5

[Rain [:rain1] (inactive)]

Attachment: updated patch including Windows support (for posterity, shouldn't be used)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 7

[Gregory Szorc [:gps]]

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.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Copying doesn't help; we need actual symlinks for developers so that dependencies on header files work properly.

Comment 9

[Gregory Szorc [:gps]]

(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

[(no longer active)]

(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.

Comment 11

[Gregory Szorc [:gps]]

I have patches in bug 1382697 that support this.