Closed Bug 208879 Opened 21 years ago Closed 21 years ago

js and xpc shells need environment reflection and argument handling cleanup

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.5alpha

People

(Reporter: brendan, Assigned: brendan)

References

()

Details

(Keywords: js1.5)

Attachments

(2 files, 9 obsolete files)

Patches coming up.

/be
Attached patch js shell patch (obsolete) — Splinter Review
This looks worse than it is because of whitespace and indentation changes. 
Apply this, but review the next one (-w).

/be
Attached patch diff -w version of last patch (obsolete) — Splinter Review
Attached patch xpcshell patch (obsolete) — Splinter Review
Again, apply this but review the next one for reduced brainstrain.

/be
Attached patch diff -w version of last patch (obsolete) — Splinter Review
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
Attachment #125283 - Attachment description: diff -w of last patch → diff -w version of last patch
Attachment #125283 - Flags: review?(shaver)
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
Attachment #125285 - Flags: review?(jband)
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.5alpha
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
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?
> 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?
Reviewers: I've also fixed env_class's name to be "environment", not "global"
(copy and paste in haste, repent at leisure).

/be
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
Attachment #125282 - Attachment is obsolete: true
Attachment #125284 - Attachment is obsolete: true
Attachment #125283 - Attachment is obsolete: true
Attachment #125285 - Attachment is obsolete: true
Attachment #125370 - Flags: superreview?(shaver)
Attachment #125370 - Flags: review?(dbradley)
Comment on attachment 125283 [details] [diff] [review]
diff -w version of last patch

Shaver's welcome to r= any of this.

/be
Attachment #125283 - Flags: review?(shaver)
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
Attachment #125285 - Flags: review?(jband)
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.
Attachment #125370 - Flags: review?(dbradley) → review+
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
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
Attachment #125369 - Attachment is obsolete: true
Attachment #125370 - Attachment is obsolete: true
Attachment #125370 - Flags: superreview?(shaver)
Attachment #125370 - Flags: review+
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
Attachment #125516 - Flags: superreview?(shaver)
Attachment #125516 - Flags: review?(dbradley)
I sure could use help testing the latest patch (attachment 125515 [details] [diff] [review]) on Windows
and other platforms.

/be
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
Does Windows support putenv, if not setenv?

/be
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
Attachment #125516 - Flags: superreview?(shaver)
Attachment #125516 - Flags: review?(dbradley)
Attachment #125515 - Attachment is obsolete: true
The latest patch allows me to build the JS shell on WinNT,
and passes the JS testsuite in both debug and optimized mode.
Fixed to avoid a double error report in the putenv (windows) case of
env_setProperty.

/be
Attachment #125540 - Attachment is obsolete: true
Attachment #125516 - Attachment is obsolete: true
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
Attachment #125557 - Flags: superreview?(shaver)
Attachment #125557 - Flags: review?(dbradley)
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.
Attachment #125557 - Flags: review?(dbradley) → review+
The latest patch passes the JS testuite on WinNT,
in both debug and optimized mode.
Fixed, I went in with r= only (shaver, feel free to sr= after the fact).

/be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
shaver, brendan: Can this sr request to shaver just be removed? It seems it
isn't required and also won't happen anymore.
Comment on attachment 125557 [details] [diff] [review]
diff -w version of last patch (review this)

Yeah, this patch landed.

/be
Attachment #125557 - Flags: superreview?(shaver)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: