339096 - Building installer should be possible under msys

Status: RESOLVED WORKSFORME

(Firefox :: Installer, defect)

Description

[Régis Caspar]

After bug 326580 landing, there is now some cygpath command used in mozilla/browser/installer/Makefile, but under msys, absolute (i.e. ala  "cygwin") paths are like "/c/path/to/foo" and not "/cygdrive/c/path/to/foo". However, msys accept and understand "c:/path/to/foo".

Will attach a patch.

Comment 1

[Régis Caspar]

Attachment: patch

Add "-m" option to the cygpath commands when we are under msys.

Comment 2

[Robert Strong (they/them - no direct email)]

Can this just be "shell cygpath -t mixed -a..." or am I missing something? Looks like xpinstall does this though I am unsure of what -ai does in addition to.
http://lxr.mozilla.org/seamonkey/source/xpinstall/packager/windows/Makefile.in#47

Comment 3

[Régis Caspar]

(In reply to comment #2)
> Can this just be "shell cygpath -t mixed -a..." or am I missing something?
-t mixed and -m are similar. From cygpath --help:
  -i, --ignore          ignore missing argument

I proposed msys detection in the patch because original code don't use -m and I don't know if it's ok under cygwin to use -m.

Comment 4

[Robert Strong (they/them - no direct email)]

Are there any other cygpath call sites in the tree (e.g. search on cygpath in lxr.mozilla.org) broken with msys as well?

Comment 5

[Régis Caspar]

(In reply to comment #4)
> Are there any other cygpath call sites in the tree (e.g. search on cygpath in
> lxr.mozilla.org) broken with msys as well?
For Firefox modules, no (I don't know for everything else because I only make Firefox builds). For this issue, if you are sure -m isn't a problem under cygwin, the patch could become trivial (just s/cygpath -a/cygpath -ma/)

Comment 6

[Robert Strong (they/them - no direct email)]

I'll download msys and verify if -m works properly with both. The introduction of the cygpath command actually happened quite some time ago well before the landing of building using NSIS for the installer so I am removing the blocks.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/installer/windows/Makefile.in&rev=1.29&root=/cvsroot

Comment 7

[Robert Strong (they/them - no direct email)]

Using -ma works fine for me with cygwin. Could you also provide a patch for mail to the same file for consistency's sake? If so, request review from mscott for that patch.

Comment 8

[Régis Caspar]

Attachment: patch v2

Patch changing "cygpath -a" to "cygpath -ma" for mail/installer/windows/Makefile.in and browser/installer/windows/Makefile.in 

For the record, http://lxr.mozilla.org/seamonkey/search?string=cygpath shows some more "-m potential" like:
/directory/c-sdk/configure.in, line 894 -- WIN_TOP_SRC=`cygpath -w $srcdir | sed -e 's|\\\\|/|g'` 
=> WIN_TOP_SRC=`cygpath -m $srcdir"
...

Comment 9

[Régis Caspar]

I found some more glitch preventing to build the installer on win32 under msys.
Patch v2 works here because I have some more file patched: there is a lot of perl script dieing if ($O =~ /msys/). Additionally, I made a builds today and after a successful build I tried making installers. I ended in a cmd shell under msys !?
I then made a lxr search on "cmd /C" (http://lxr.mozilla.org/seamonkey/search?string=cmd+%2FC) and found some results. I don't think the building process should require cmd to work, why not rather use cygwin or msys shell (i.e. sh)? 
So there is two possibilities, either msys support is added or it is discarded and this bug would becomes INVALID. 

I will cancel review on patch 2 for now, update summary and attach a diff.

Comment 10

[Régis Caspar]

Attachment: diff (cf. comment #9)

Comment 11

[Robert Strong (they/them - no direct email)]

I think this would be a good thing to do but I'd like to audit the places that use cygpath, cmd.exe (if there are any others) throughout the tree and make sure they all work with msys vs. fixing them in multiple patches. If someone took that on I'd be glad to review, help get the module owners to review, etc. Also, I have read that accessibility doesn't build without msvc and I'd like to know how / if the NSIS installer handles that properly or should it also be handled differently.

Comment 12

[Régis Caspar]

(In reply to comment #11)
> I think this would be a good thing to do but I'd like to audit the places that
> use cygpath, cmd.exe (if there are any others) throughout the tree and make
> sure they all work with msys vs. fixing them in multiple patches. 
There's only a few cmd in the tree. Most are "handled" in the diff I added. All build fine here (msys / VC8) the only tests I didn't made is the locales repackaging (browser|mail)/locales/Makefile.in, a TB installer build and cat -B file-1 file-2 > file under cygwin (I don't have it). 
Here I don't want to point at cygpath usage, it's a requirement and that's OK, but about cmd!? we have a "powerful" shell (sh) but we use another "simple" one (cmd). In a more critical point of view why a perl script require the use of a windows batch??

> If someone took that on I'd be glad to review, help get the module owners to 
> review, etc. 
OK, most of the msys support is already in the diff I attached. I'd be glad to help/take it but I will probably need some direction.

 -> reassign to default

Comment 13

[Régis Caspar]

(In reply to comment #12)
> the only tests I didn't made is the locales repackaging 
> (browser|mail)/locales/Makefile.in, a TB installer build and cat -B
> file-1 file-2 > file under cygwin (I don't have it). 
And I didn't test anything under OS2 (I don't have it either)

Comment 14

[Benjamin Smedberg]

What would you replace cmd /C copy /b with? Why doesn't "cmd" work with MSYS?

Comment 15

[Régis Caspar]

(In reply to comment #14)
> What would you replace cmd /C copy /b with? 
cat -B

> Why doesn't "cmd" work with MSYS?
That's a good question. The exact problem is that cmd doesn't exit back to sh, so you end into a cmd session

Note:
$ sh --version
GNU bash, version 2.04.0(1)-release (i686-pc-msys)
Copyright 1999 Free Software Foundation, Inc.

Comment 16

[Robert Strong (they/them - no direct email)]

Regis, the build config for the installer has changed quite a lot since this bug was reported. Is this still a problem?

Comment 17

[Régis Caspar]

(In reply to comment #16)
> Regis, the build config for the installer has changed quite a lot since this
> bug was reported. Is this still a problem?
I will check and report. If the diff (third attachment) is correct, the problem still exists ($^O = 'msys' under MSYS and cmd.exe cannot be "cast").

BTW:
(In reply to comment #15)
> > What would you replace cmd /C copy /b with? 
> cat -B
the -B is a win32 only flag which is useless here 

Comment 18

[Régis Caspar]

(In reply to comment #17)
> I will check and report. 
Robert, I can build dist/install/sea/firefox-3.0a1.en-US.win32.installer.exe with make -C browser/installer installer without any problem. The content of the built one match the content (file names / directories) of the official nightly installer. So this seems fixed. 

 -> WORKSFORME