nsinstall.py is so much easier to maintain. This seems to fix a bunch of bugs, including the nasty bug 752202.

Comment on attachment 625797 [details] [diff] [review] patch for c-c Closely tied to the m-c version of this patch, will let ted r+ given that

Comment on attachment 625797 [details] [diff] [review] patch for c-c Review of attachment 625797 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/config.mk @@ +620,5 @@ > endif # OS2 > endif # NSINSTALL_BIN > > ifeq (,$(CROSS_COMPILE)$(filter-out WINNT OS2, $(OS_ARCH))) > +INSTALL = $(NSINSTALL) -t Nit: it looks like you wanted to remove spaces before '='. (In the 2 other config.mk too.)

nsinstall.py changed.

I can confirm that these patches apply cleanly and allow me to build on Windows again.

Moving to blocker because of bug 752202

Comment on attachment 625797 [details] [diff] [review] patch for c-c Review of attachment 625797 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/config.mk @@ +637,1 @@ > DIR_INSTALL = $(INSTALL) We should probably do a followup to just s/DIR_INSTALL/INSTALL/.

Comment on attachment 627962 [details] [diff] [review] Updated patch to trunk Review of attachment 627962 [details] [diff] [review]: ----------------------------------------------------------------- r- for this Unicode issue, but otherwise looks pretty good. ::: config/nsinstall.py @@ +54,5 @@ > (options, args) = p.parse_args(argv) > + # Switching to Unicode strings makes python use the wide Windows APIs, which is > + # what we want here since the wide APIs normally do a better job at handling long > + # paths and such. > + args = map(unicode, args) This worries me quite a bit. Passing each arg to unicode() means they'll be decoded as ASCII. Presumably this is different than the previous behavior, where they'd be handled in the native codepage. This breaks on Linux, for example: python -c "import sys; print unicode(sys.argv[1])"

This addresses Ted's comments by fixing issues with - Unicode on Unixes, via properly decoding sys.argv - Unicode on Windows, via using ctypes/GetCommandLineW. This also fixes a broken invocation of nsinstall by moving over a whole bunch of files, hence the size of the patch. Ted's review comment was truncated by his brave attempt to use non-BMP characters in the bugzilla text field :) > @@ +65,5 @@ > > sys.stderr.write('nsinstall: ' + options.m + ' is not a valid mode\n') > > return 1 > > > > # just create one directory? > > + def maybe_create_dir(dir, mode, try_again): > I'd just drop the try_again parameter and stick this in a loop. Probably > something similar to: > http://mxr.mozilla.org/mozilla-central/source/config/utils.py#23 I tried doing this but couldn't do it in a satisfactory way. The utils.py example is simpler, so a loop works there. Here, I want to run some code again that isn't covered by the try block, and I don't want it to be covered by the try block either. I've pushed this patch to try: https://tbpl.mozilla.org/?tree=Try&rev=944d55c07da4

Comment on attachment 633197 [details] [diff] [review] Patch for m-c, v2 Review of attachment 633197 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/config.mk @@ +662,1 @@ > DIR_INSTALL = $(INSTALL) Should probably just s/DIR_INSTALL/INSTALL/ in a followup. ::: config/nsinstall.py @@ +168,5 @@ > + > + argc = ctypes.c_int(0) > + argv_arr = CommandLineToArgv(GetCommandLine(), ctypes.byref(argc)) > + # The first argument will be "python", the second will be the .py file > + argv = [argv_arr[i] for i in xrange(1, argc.value)] You can't slice this because it's some goofy ctypes-type? ::: config/tests/unit-nsinstall.py @@ +134,5 @@ > + self.assertEqual(nsinstall([testfile.encode("utf-8"), > + testdir.encode("utf-8")]), 0) > + > + destfile = os.path.join(testdir, filename) > + self.assert_(os.path.isfile(destfile)) A more thorough test here would involve actually executing nsinstall.py via subprocess, since otherwise you're not testing that block of code in __main__.

I've addressed the review comments, but I'm still investigating an OS X-only failure with the subprocess test on the try server that I can't reproduce locally...

Turns out our tinderboxes really hate Unicode. I've disabled the subprocess test on platforms where Unicode isn't enabled.

This is blocked on bug 769808 for now. I'll land as soon as that is fixed.

Filed bug 769907 to figure out why our Mac and Linux build envs aren't set to UTF-8.

Filed bug 769908 to get rid of DIR_INSTALL.

http://hg.mozilla.org/mozilla-central/rev/e149b8c85eb8 http://hg.mozilla.org/comm-central/rev/fd6aa13b1c83

nsinstall.exe.manifest is unnecessary after fixing this.