Last Comment Bug 757252 - Kill nsinstall_win.c
: Kill nsinstall_win.c
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows 7
: -- blocker (vote)
: mozilla16
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
:
Mentors:
Depends on: 769789 769808 770558
Blocks: 680636 752202 769908
  Show dependency treegraph
 
Reported: 2012-05-21 15:40 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-07-03 09:05 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch for m-c (56.53 KB, patch)
2012-05-21 15:40 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
patch for c-c (1.81 KB, patch)
2012-05-21 15:43 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review+
Details | Diff | Splinter Review
Updated patch to trunk (57.29 KB, patch)
2012-05-29 07:55 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review-
Details | Diff | Splinter Review
Patch for m-c, v2 (210.86 KB, patch)
2012-06-14 11:14 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review+
Details | Diff | Splinter Review
what I'm going to check in (m-c) (208.92 KB, patch)
2012-06-30 03:48 PDT, Siddharth Agarwal [:sid0] (inactive)
sid.bugzilla: review+
Details | Diff | Splinter Review
what I'm going to check in (c-c) (1.89 KB, patch)
2012-06-30 04:07 PDT, Siddharth Agarwal [:sid0] (inactive)
sid.bugzilla: review+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2012-05-21 15:40:39 PDT
Created attachment 625794 [details] [diff] [review]
patch for m-c

nsinstall.py is so much easier to maintain. This seems to fix a bunch of bugs, including the nasty bug 752202.
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2012-05-21 15:43:00 PDT
Created attachment 625797 [details] [diff] [review]
patch for c-c
Comment 2 Justin Wood (:Callek) 2012-05-21 15:44:40 PDT
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 3 Serge Gautherie (:sgautherie) 2012-05-21 18:06:18 PDT
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.)
Comment 4 Siddharth Agarwal [:sid0] (inactive) 2012-05-29 07:55:01 PDT
Created attachment 627962 [details] [diff] [review]
Updated patch to trunk

nsinstall.py changed.
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2012-06-05 14:00:30 PDT
I can confirm that these patches apply cleanly and allow me to build on Windows again.
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2012-06-06 12:51:44 PDT
Moving to blocker because of bug 752202
Comment 7 Ted Mielczarek [:ted.mielczarek] 2012-06-07 09:44:27 PDT
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 8 Ted Mielczarek [:ted.mielczarek] 2012-06-07 10:20:41 PDT
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])"
Comment 9 Siddharth Agarwal [:sid0] (inactive) 2012-06-14 11:14:58 PDT
Created attachment 633197 [details] [diff] [review]
Patch for m-c, v2

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 10 Ted Mielczarek [:ted.mielczarek] 2012-06-29 11:28:00 PDT
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__.
Comment 11 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 01:54:05 PDT
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...
Comment 12 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 03:48:46 PDT
Created attachment 638093 [details] [diff] [review]
what I'm going to check in (m-c)

Turns out our tinderboxes really hate Unicode. I've disabled the subprocess test on platforms where Unicode isn't enabled.
Comment 13 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 03:50:17 PDT
This is blocked on bug 769808 for now. I'll land as soon as that is fixed.
Comment 14 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 04:07:31 PDT
Created attachment 638095 [details] [diff] [review]
what I'm going to check in (c-c)
Comment 15 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 04:23:39 PDT
Filed bug 769907 to figure out why our Mac and Linux build envs aren't set to UTF-8.
Comment 16 Siddharth Agarwal [:sid0] (inactive) 2012-06-30 04:28:56 PDT
Filed bug 769908 to get rid of DIR_INSTALL.
Comment 18 Makoto Kato [:m_kato] 2012-07-03 00:57:05 PDT
nsinstall.exe.manifest is unnecessary after fixing this.

Note You need to log in before you can comment on or make changes to this bug.