243616 - Error: 'File not found: nsinstall.exe' when building xpinstall using build.pl

Status: RESOLVED FIXED

(SeaMonkey :: Installer, defect)

Description

[Andreas Kunz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040501
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040501

[The reporter of bug 215864 did not want to own that bug anymore and resolved
it. This is the replacement and the first few contents of this description are
pasted (and edited) from various comments of that other bug (originally not
written by me):]

When trying to build the Windows XPinstall 'wizard', via 'perl build.pl', it
gets some way, and then aborts with the error message box 'file not found:
nsinstall.exe'.
This file nsinstall.exe _does_ exist where it should, thoug.

(Building on Windows XP for the Windows XPinstall wizard, using Cygwin Perl
[ActiveState Perl also does not work].
Some people say that [Cygwin Perl] has problems but one person told me they
regularly
used Cygwin Perl to build... so it shouldn't really be a problem.)

Reproducible: Always
Steps to Reproduce:
1. Try and 'perl build.pl' in mozilla/xpinstall/wizard/windows/builder

Actual Results:  
Aborted

Expected Results:  
Completed

Error messages on console are:
===
building self-extracting uninstaller (GREUninstall.exe)...

 Error: /cygdrive/c/games/Moz/mozilla/dist/inst_gre/../install/nsztool.exe /cygd
rive/c/games/Moz/mozilla/dist/inst_gre/GREUninstall.exe /cygdrive/c/games/Moz/mo
zilla/dist/inst_gre/uninstall/*.*

 Error: perl "/cygdrive/c/games/Moz/mozilla/xpinstall/packager/win_gre/makeall.p
l" -objDir "/cygdrive/c/games/Moz/mozilla" -stagePath "/cygdrive/c/games/Moz/moz
illa/stage" -distPath "/cygdrive/c/games/Moz/mozilla/dist" -aurl ftp://not.suppl
ied.invalid -rurl ftp://not.supplied.invalid

 Error: perl makeall.pl -objDir "/cygdrive/c/games/Moz/mozilla" -stagePath "/cyg
drive/c/games/Moz/mozilla/stage" -distPath "/cygdrive/c/games/Moz/mozilla/dist"
-aurl ftp://not.supplied.invalid -rurl ftp://not.supplied.invalid
===

This happens at least on WinXP with VC.NET 2003 and on Win2k with VC6.

In line 504 of makeall.pl the following fails:
if(system("/cygdrive/k/mozdev/objdir.NET-debug-von_o4/dist/inst_gre/../install/nsztool.exe
$gDirDistInstall/$seuFileNameSpecific $gDirDistInstall/uninstall/*.*"))

It calls nsztool.exe and this program seems not to understand cygwin paths; in
line 659:
    if(!FileExists(szSeaExe))
evaluates to "file does not exist", although that file _does_ exist (szSeaExe
being "/cygdrive/k/mozdev/mozilla/dist/inst_gre/GREUninstall.exe").

When szSeaExe is manually set to
"K:\\mozdev\\mozilla\\dist\\inst_gre\\GREUninstall.exe" before, it evaluates to
"file does exist" (and a different error occurs later, namely after line 993).

FileExists resides also in nsztool.c and basically does:
     if((rv = GetFileAttributes(szFile)) == -1)
"GetFileAttributes" is a Windows API method declared in winbase.h. This header
file is brought by VisualStudio AND by Cygwin. But forcing it to use the Cygwin
one does not help, I guess the implementation is that one of Windows anyway.

So I guess the fix would be to make FileExists also recognize and check Cygwin
paths correctly. But apart from the guess that other parts of the Mozilla build
system also have to handle cygwin paths, I have no idea how to gon on from here.

Comment 1

[timeless]

no the fix is to run cygpath on the parameters

Comment 2

[Andreas Kunz]

Attachment: patch

Whatever you say.
This patch fixes this bug by calling nsztool.exe with "windowsized" paths in
the arguments from makeall.pl in 'win_gre' and 'windows'.

There is also a makeall.pl in 'win_mfcembed' that is not used for the standard
installer, but does about the same thing and probably could be fixed the same
way.
Using 'build_mfcembed.pl' to test it shows the fix does also work, but this
script fails at about its final stage and I'm not sure if there is a connection
to this patch. It's also not the scope of this bug, so I'll leave it to
somebody else as an exercise...

A note to all people having separate objdirs: build.pl seems to assume that
obrdir=srcdir, at least I had to hardcode my objdir in about the 12th line of
build.pl to make it work.

Comment 3

[Andreas Kunz]

Comment on attachment 148539 [details] [diff] [review]
patch

Asking bsmedberg to have a look at it.

In bug 215864 comment #1 you said that you don't have this problem and ssu also
does not seem to have it (somehow the windows installer builds _are_ created,
no doubt...). Having looked at the code I really wonder how it can work for
you... maybe it's somehow already getting windows-style paths.

Does it still work with this patch?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #2)
> A note to all people having separate objdirs: build.pl seems to assume that
> obrdir=srcdir, at least I had to hardcode my objdir in about the 12th line of
> build.pl to make it work.

perl build.pl -objdir c:/objdir

Comment 5

[Andreas Kunz]

Uh... when giving the objdir path this way and in Windows-style, this bug does
not occur...
Is it even invalid?
But no, when calling it without parameters, it gets a Cygwin-style path as
topsrcdir (and due to the lack of an objdir also as objdir) and this bug _does_
occur.
Of course one could work around the problem by explicitly specifying the objdir
to be identical to the topsrcdir - in Windows style, but...

It's up to you...

Comment 6

[Benjamin Smedberg]

Comment on attachment 148539 [details] [diff] [review]
patch

I don't like changing the variable name. Please do

$gDirDistInstall = `cygpath -mai $inDistPath/inst_gre`;
chomp $gDirDistInstall;

Comment 7

[Andreas Kunz]

Attachment: new patch

New and simpler patch with review comments incorporated.
Tested, works fine.

Comment 8

[Andreas Kunz]

Comment on attachment 150311 [details] [diff] [review]
new patch

Requesting r= from bsmedberg.

Comment 9

[Benjamin Smedberg]

Comment on attachment 150311 [details] [diff] [review]
new patch

r=bsmedberg (no sr needed for build-config changes)

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in xpinstall/packager/win_gre/makeall.pl;
/cvsroot/mozilla/xpinstall/packager/win_gre/makeall.pl,v  <--  makeall.pl
new revision: 1.13; previous revision: 1.12
done
Checking in xpinstall/packager/windows/makeall.pl;
/cvsroot/mozilla/xpinstall/packager/windows/makeall.pl,v  <--  makeall.pl
new revision: 1.71; previous revision: 1.70
done

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Tinderbox has to say:
copy /../install/ds32.exe : No such file or directory
...
 Error: perl
"c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpinstall/packager/win_gre/makeall.pl"
-objDir "c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla" -stagePath
"c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/stage" -distPath
"c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dist" -aurl
http://ftp.mozilla.org/pub/mozilla.org/mozilla/tinderbox-builds/CREATURE/windows-xpi
-rurl
http://ftp.mozilla.org/pub/mozilla.org/mozilla/tinderbox-builds/CREATURE/windows-xpi







http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1089928680.30835.gz&fulltext=1

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

and because that caused tinderbox orange, I backed the patch out

Comment 13

[Andreas Kunz]

I think I found the problem in the long build log: the "cygpath" version on
creature does not know the "-m" command line option:

   cygpath: unknown option -- m
   Usage: cygpath (-u|-w|-t TYPE) [-c HANDLE] [-f FILE] [options] NAME

Instead we shoud obviously use "--type mixed", the long form.
I'll post an updated patch when I've tested it (might take several days).

Comment 14

[Jon Granrose]

would it make sense to update cygwin on creature, or is that more risk than it's
worth?  i.e. are people building seamonkey with the latest cygwin and able to
make the installers while running from a cygwin shell using this patch?

Comment 15

[Jon Granrose]

fwif, I was trying to update the tinderbox scripts on creature for 266291
(talkback not working) and ran into this bug and had to revert.

Comment 16

[Andreas Kunz]

Ooops, sorry that I didn't post the promised patch; unfortunately I won't have
the time to test it during the next weeks.
But it should be very easy to change my patch according to comment #13. I don't
know if that old cygpath version supports "mixed" at all and if other options
would also work.
The best is probably to try "-t mixed" first.

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: (hopefully) better patch

uses -t mixed instead of -m

Comment 18

[Benjamin Smedberg]

Comment on attachment 181210 [details] [diff] [review]
(hopefully) better patch

I don't think we should be using cygpath in the perl scripts. Use it in the
makefile if you must.

Comment 19

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

This should have been fixed by the checkin for bug 290905.