Open Bug 680968 Opened 13 years ago Updated 2 years ago

Support symlinks in nsinstall.py

Categories

(Firefox Build System :: General, defect)

defect

Tracking

(Not tracked)

People

(Reporter: gps, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(3 obsolete files)

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.
I think I fat fingered the bug number when I created this bug.
Blocks: 680636
No longer depends on: 680536
Attached patch Basic symlink support in nsinstall.py (obsolete) (deleted) — Splinter Review
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)
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)
Ugh, this is never going to work on Windows. rm -rf follows junctions :(
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.
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.
(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.
(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.
Product: Core → Firefox Build System
Attachment #596207 - Attachment is obsolete: true
Assignee: sid.bugzilla → nobody
Status: ASSIGNED → NEW
I have patches in bug 1382697 that support this.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: