Closed Bug 757252 Opened 11 years ago Closed 11 years ago

Kill nsinstall_win.c


(Firefox Build System :: General, defect)

Windows 7
Not set


(Not tracked)



(Reporter: rain1, Assigned: rain1)




(2 files, 4 obsolete files)

Attached patch patch for m-c (obsolete) — Splinter Review is so much easier to maintain. This seems to fix a bunch of bugs, including the nasty bug 752202.
Attachment #625794 - Flags: review?(ted.mielczarek)
Attached patch patch for c-c (obsolete) — Splinter Review
Attachment #625797 - Flags: review?(bugspam.Callek)
Attachment #625797 - Attachment is patch: true
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
Attachment #625797 - Flags: review?(bugspam.Callek) → review?(ted.mielczarek)
Comment on attachment 625797 [details] [diff] [review]
patch for c-c

Review of attachment 625797 [details] [diff] [review]:

::: config/
@@ +620,5 @@
>  endif # OS2
>  endif # NSINSTALL_BIN
>  ifeq (,$(CROSS_COMPILE)$(filter-out WINNT OS2, $(OS_ARCH)))

Nit: it looks like you wanted to remove spaces before '='. (In the 2 other too.)
Attached patch Updated patch to trunk (obsolete) — Splinter Review changed.
Attachment #625794 - Attachment is obsolete: true
Attachment #625794 - Flags: review?(ted.mielczarek)
Attachment #627962 - Flags: review?(ted.mielczarek)
Blocks: 680636
I can confirm that these patches apply cleanly and allow me to build on Windows again.
Moving to blocker because of bug 752202
Severity: normal → blocker
Comment on attachment 625797 [details] [diff] [review]
patch for c-c

Review of attachment 625797 [details] [diff] [review]:

::: config/
@@ +637,1 @@

We should probably do a followup to just s/DIR_INSTALL/INSTALL/.
Attachment #625797 - Flags: review?(ted.mielczarek) → review+
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/
@@ +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])"
Attachment #627962 - Flags: review?(ted.mielczarek) → review-
Attached patch Patch for m-c, v2 (obsolete) — Splinter Review
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:

I tried doing this but couldn't do it in a satisfactory way. The 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:
Attachment #627962 - Attachment is obsolete: true
Attachment #633197 - Flags: review?(ted.mielczarek)
Comment on attachment 633197 [details] [diff] [review]
Patch for m-c, v2

Review of attachment 633197 [details] [diff] [review]:

::: config/
@@ +662,1 @@

Should probably just s/DIR_INSTALL/INSTALL/ in a followup.

::: config/
@@ +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/
@@ +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 via subprocess, since otherwise you're not testing that block of code in __main__.
Attachment #633197 - Flags: review?(ted.mielczarek) → review+
Depends on: 769789
Depends on: 769808
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.
Attachment #633197 - Attachment is obsolete: true
Attachment #638093 - Flags: review+
This is blocked on bug 769808 for now. I'll land as soon as that is fixed.
Attachment #625797 - Attachment is obsolete: true
Attachment #638095 - Flags: review+
Filed bug 769907 to figure out why our Mac and Linux build envs aren't set to UTF-8.
See Also: → 769907
Filed bug 769908 to get rid of DIR_INSTALL.
See Also: → 769908
Blocks: 769908
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
nsinstall.exe.manifest is unnecessary after fixing this.
Depends on: 770558
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.