Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Kill nsinstall_win.c

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
--
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sid0, Assigned: sid0)

Tracking

Trunk
mozilla16
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
Attachment #625794 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 1

5 years ago
Created attachment 625797 [details] [diff] [review]
patch for c-c
Attachment #625797 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

5 years ago
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/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.)
(Assignee)

Comment 4

5 years ago
Created attachment 627962 [details] [diff] [review]
Updated patch to trunk

nsinstall.py changed.
Attachment #625794 - Attachment is obsolete: true
Attachment #625794 - Flags: review?(ted.mielczarek)
Attachment #627962 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Blocks: 680636
I can confirm that these patches apply cleanly and allow me to build on Windows again.
(Assignee)

Comment 6

5 years ago
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/config.mk
@@ +637,1 @@
>  DIR_INSTALL = $(INSTALL)

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/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])"
Attachment #627962 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 9

5 years ago
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
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/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__.
Attachment #633197 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

5 years ago
Depends on: 769789
(Assignee)

Updated

5 years ago
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...
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.
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.
Created attachment 638095 [details] [diff] [review]
what I'm going to check in (c-c)
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: → bug 769907
Filed bug 769908 to get rid of DIR_INSTALL.
See Also: → bug 769908
(Assignee)

Updated

5 years ago
Blocks: 769908
http://hg.mozilla.org/mozilla-central/rev/e149b8c85eb8
http://hg.mozilla.org/comm-central/rev/fd6aa13b1c83
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
nsinstall.exe.manifest is unnecessary after fixing this.
Depends on: 770558
You need to log in before you can comment on or make changes to this bug.