Closed Bug 463417 Opened 12 years ago Closed 12 years ago

import win32 nsinstall source into mozilla-central, build by default on win32

Categories

(Firefox Build System :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: ted, Assigned: benjamin)

References

()

Details

Attachments

(2 files)

We currently ship nsinstall.exe with MozillaBuild, and the win32 version isn't in mozilla-central. This is probably just for historical reasons. The down side is that if we find a bug in nsinstall, like bug 463411, it's very hard to fix. I don't see any reason we shouldn't just import the source and build it by default on win32. We will obviously have to account for the mingw cross-compile case, since the host won't be able to compile the native nsinstall there.

cls: do you know of any other pitfalls we might hit here?
The host would be able to compile the unixy nsinstall.c, though, wouldn't it?
Yes, and Neil says we already do in that case.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #387684 - Flags: review?
Attachment #387684 - Flags: review? → review?(timeless)
Comment on attachment 387684 [details] [diff] [review]
Build nsinstall.exe as part of the builds, rev. 1

btw, i'm perfectly fine with you checking in a patch series (in fact, i think it's a good idea), the original and the one for review comments.

That way someone can more easily compare with the original.

Among other things, nsinstall's code style /generally/ includes a space between function and ( arguments ) ( and inside parens too ), which isn't typical for modern Gecko style.

>diff --git a/config/nsinstall_win.c b/config/nsinstall_win.c

>+ * The nsinstall command for Win32

most of this block is legacy, and could go for a good spring cleaning :)

>+/* changes all forward slashes in token to back slashes */

google: did you mean backslashes :)

>+void changeForwardSlashesTpBackSlashes ( wchar_t *arg )

>+int wmain(int argc, wchar_t *argv[ ])
over-indented:
>+        return shellNsinstall ( argv + 1 );

>+shellNsinstall (wchar_t **pArgv)

tabs :(
>+    int retVal = 0;		/* exit status */
>+    int retVal = 0;			/* assume valid return */

please fix the spelling :)
>+            /* check if directory alreay exists */
Attachment #387684 - Flags: review?(timeless) → review+
Depends on: 505289
Depends on: 505574
Comment on attachment 387684 [details] [diff] [review]
Build nsinstall.exe as part of the builds, rev. 1

>-MOZ_PATH_PROGS(NSINSTALL_BIN, nsinstall )
Fortunately comm-central's configure.in still autodetects my custom nsinstall in the path... if I didn't have that luxury would it still be possible for me to use my custom version?
Ah, I figured it out, it will pick it up from the $NSINSTALL_BIN env variable.
Blocks: songbird
Target Milestone: --- → mozilla1.9.2b1
Target Milestone: mozilla1.9.2b1 → mozilla1.9.2a1
Fwiw, (I assume it was from a repository sync')
mozilla1.9.3a1:
http://hg.mozilla.org/mozilla-central/rev/b5822dea95a4
import the nsinstall.exe source from CVS buildtools repository: make it capable of copying files with very long names, build it by default on Windows hosts, and stop using the moztools version
Attachment #428638 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 428638 [details] [diff] [review]
(Bv1-CC) Copy (the useful part of) it to comm-central, Copy bug 505574 too
[Checkin: Comment 10]


http://hg.mozilla.org/comm-central/rev/af3aee23f01e
Attachment #428638 - Attachment description: (Bv1-CC) Copy (the useful part of) it to comm-central, Copy bug 505574 too → (Bv1-CC) Copy (the useful part of) it to comm-central, Copy bug 505574 too [Checkin: Comment 10]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.