208879 - js and xpc shells need environment reflection and argument handling cleanup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Patches coming up.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: js shell patch

This looks worse than it is because of whitespace and indentation changes. 
Apply this, but review the next one (-w).

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 3

[Brendan Eich [:brendan]]

Attachment: xpcshell patch

Again, apply this but review the next one for reduced brainstrain.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 125283 [details] [diff] [review]
diff -w version of last patch

Normally these files don't need review, but in this case I'd appreciate a few
eyeball-minutes.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 125285 [details] [diff] [review]
diff -w version of last patch

This too could use an r=, even though it's not part of Mozilla-the-binary.

/be

Comment 7

[Brendan Eich [:brendan]]

Not that it matters, except for leak detection, but ProcessArgs neglects to free
fargv if it is non-null.  I patched my tree to free it (at the bottom, after the
for (i = 0; i < nf; i++) and all -- for-loop and free call -- only if (fargv)),
but I won't bother y'all with new patches to review.

/be

Comment 8

[rogerl (gone)]

Not that it's particularly relevant to your patch (which I tested a little on
Win32, all fine) but I'm pretty sure that all the code inside #ifdef
MAC_TEST_HACK can get removed now. It was something I cobbled together for
Christine to get the tests running on a Mac, way back. I don't believe the shell
is ever built that way anymore. Phil?

Comment 9

[Phil Schwartau]

> I don't believe the shell is ever built that way anymore.

I don't, at any rate - it's my understanding that Mac Classic
is no longer supported.

As for MAC_TEST_HACK, note there are other flags of the same ilk,
I believe. Such as XP_MAC_MPW; see bug 62187 comment 1.

Should I open a separate bug for removing all Mac Classic items?

Comment 10

[Brendan Eich [:brendan]]

Reviewers: I've also fixed env_class's name to be "environment", not "global"
(copy and paste in haste, repent at leisure).

/be

Comment 11

[Brendan Eich [:brendan]]

Phil: please do open separate bug(s), I'm not gonna try to clean up Mac ifdefs
in this bug.  Just trying to get these landed today.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: latest and greatest, all-in-one patch (apply this)

Comment 13

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch (review this)

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 125283 [details] [diff] [review]
diff -w version of last patch

Shaver's welcome to r= any of this.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 125285 [details] [diff] [review]
diff -w version of last patch

jband is also welcome to review anything in the latest patch.

/be

Comment 16

[David Bradley]

Comment on attachment 125370 [details] [diff] [review]
diff -w version of last patch (review this)

r=dbradley

There's some behavior change here, but I think it can be considered positive.
Arguments used to only apply to the last script, and not the scripts specified
with -f. Also it was possible to specify different versions, strictness, etc.
for each file. I doubt anyone is taking advantage of that.

Comment 17

[David Bradley]

Oh, one last thing I just thought of. I don't think it would be too much trouble
to get this working on Windows. Add a #ifdef's that adds the envp parameter to
main, and then assigns it to environp. Something like below, I don't remember
what the appropriate macro for win is. Then adjust the XP_UNIX #ifdef's accordingly.

// Somewhere up top
#ifdef XP_WIN
char ** environp;
#endif

#ifdef XP_WIN
int
main(int argc, char **argv, char **envp)
{
   environp = envp;
#else
int
main(int argc, char **argv)
{
#endif

Comment 18

[Brendan Eich [:brendan]]

dbradley: yeah, I wanted arguments to be available to the -f file Process()
calls. That was a goal. I didn't think too much about the loss of ability to
interleave -v options with -f options, though. Maybe that's a bigger loss than
it seems?

To fix that, we could make two passes over the optional arguments, one to find
the remaining non-optional ones to put in the arguments object, the second to
process the -v, -f, etc. options, then the final script-file, if any.

Thanks for the windows tip -- that works on Unix, too, so I'll just expose it on
all platforms and let Mac users test it (probably works on OS X).

This is worth getting right.  I'll hack a bit more.

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: better fix, thanks to dbradley's comments (apply this)

Comment 20

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch (review this)

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 125516 [details] [diff] [review]
diff -w version of last patch (review this)

dbradley: I also renamed jscontext in xpcshell's main to cx, so as to make it
easier to re-unify the shells, per the bug you just filed.

/be

Comment 22

[Brendan Eich [:brendan]]

I sure could use help testing the latest patch (attachment 125515 [details] [diff] [review]) on Windows
and other platforms.

/be

Comment 23

[Phil Schwartau]

Am I doing something wrong? I get a build error on WinNT after
applying the latest patch ("better fix, .... (apply this)").

It occurs on the linking step. I'm building via Makefile.ref:

                 etc.
                 etc.

Creating library WINNT4.0_DBG.OBJ/js32.lib and object WINNT4.0_DBG.OBJ/js32.exp
cl -FoWINNT4.0_DBG.OBJ/ -c /MD /Od /Z7  -D_X86_=1 -DXP_WIN -DXP_WIN32 -DWIN32
-D_WINDOWS -D_WIN32 /nologo /W3 /FpWINNT4.0_DBG.
OBJ/js.pch  -DDEBUG -DDEBUG_pschwartau   /Op js.c
js.c
js.c(2012) : warning C4013: 'setenv' undefined; assuming extern returning int
js.c(2020) : warning C4273: '__p__environ' : inconsistent dll linkage. 
dllexport assumed.
link.exe -out:"WINNT4.0_DBG.OBJ/js.exe" kernel32.lib user32.lib gdi32.lib
winspool.lib comdlg32.lib advapi32.lib shell32.lib o
le32.lib oleaut32.lib uuid.lib oldnames.lib /nologo /subsystem:console /debug
/pdb:none /machine:I386 /opt:ref /opt:noicf WINN
T4.0_DBG.OBJ/js.obj WINNT4.0_DBG.OBJ/js32.lib fdlibm/WINNT4.0_DBG.OBJ/fdlibm.lib
js.obj : error LNK2001: unresolved external symbol _setenv
WINNT4.0_DBG.OBJ/js.exe : fatal error LNK1120: 1 unresolved externals
make[1]: *** [WINNT4.0_DBG.OBJ/js.exe] Error 96
make[1]: Leaving directory `//d/JS_EXP/mozilla/js/src'
make: *** [all] Error 2

Comment 24

[Brendan Eich [:brendan]]

Does Windows support putenv, if not setenv?

/be

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 125516 [details] [diff] [review]
diff -w version of last patch (review this)

Got a patch that uses putenv on Windows.  Hope that API exists.  Would be
better if it copied its argument, rather than making it part of the env.  Can
someone check?

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: better attempt at Windows environment portability (apply this)

Comment 27

[Phil Schwartau]

The latest patch allows me to build the JS shell on WinNT,
and passes the JS testsuite in both debug and optimized mode.

Comment 28

[Brendan Eich [:brendan]]

Attachment: best patch (apply this)

Fixed to avoid a double error report in the putenv (windows) case of
env_setProperty.

/be

Comment 29

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch (review this)

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 125557 [details] [diff] [review]
diff -w version of last patch (review this)

Ok, this is ready to go into 1.5, I think.

/be

Comment 31

[David Bradley]

Comment on attachment 125557 [details] [diff] [review]
diff -w version of last patch (review this)

r=dbradley

The modifying of the envp strings caused me a little concern, but you put it
back the way it was, so I'm fine with that.

Comment 32

[Phil Schwartau]

The latest patch passes the JS testuite on WinNT,
in both debug and optimized mode.

Comment 33

[Brendan Eich [:brendan]]

Fixed, I went in with r= only (shaver, feel free to sr= after the fact).

/be

Comment 34

[hacker formerly known as [e-mail]]

This is causing a world of trouble on half of the ports boxes.  Apparently,
solaris, beos, os2 & hpux do not have setenv() either.  For the Mozilla side of
things, we should probably add a configure test for setenv and maybe putenv if
more than just HPUX has a broken version.  Or we could skip that and use
PR_SetEnv for the xpcshell changes since xpcshell already depends upon xpcom & nspr.

Comment 35

[Brendan Eich [:brendan]]

I just checked in an ugly #if !defined XP_BEOS && !defined XP_OS2 && !defined
SOLARIS ... #endif aroung the guts of env_setProperty. Anyone who feels
motivated to port env_setProperty to those three platforms, feel free.  I don't
think this is worth an autoconf test right now.  Maybe when we revisit the code
and try to unify shell source a bit.

/be

Comment 36

[Frank Wein [:mcsmurf]]

shaver, brendan: Can this sr request to shaver just be removed? It seems it
isn't required and also won't happen anymore.

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 125557 [details] [diff] [review]
diff -w version of last patch (review this)

Yeah, this patch landed.

/be