227500 - (nsIEnvironment) Provide Scriptable access to set environment

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

Current nsIProcess provides a scriptable method to read the environment, but not
to set the environment. I would like scriptable methods to set the environment
(so that I don't have to create a binary component for a component that is
otherwise completely JS+shell scripts). In addition, the requirements that
NS_SetEnv strings be of static duration makes these difficult to code.

I'm doing a small nsIEnvironment interface that we can freeze soon.

Comment 1

[Benjamin Smedberg]

Attachment: new interface nsIEnvironment

Comment 2

[Benjamin Smedberg]

Comment on attachment 136819 [details] [diff] [review]
new interface nsIEnvironment

timeless, could you review this first? I know you have thought about the
issues; then I'll get dougt to give approval.

Comment 3

[Roland Mainz]

Before you freeze the interface please add two more APIs:
1. An enumerator to list all current environment variables.
2. There should be an observer mechanism that the code within Mozills can track
env variable changes.

Env var strings are encoded in the current locale and it is valid to have chars
outside the ASCII range (so PromiseFlagCString isn't the correct way... jshin -
can you help ?)
JS unicode strings should be converted to the current locale (e.g. $LANG on
Unix/Linux; for example "ja_JP.PCK" (which means the UTF-8 conversion functions
don't work here... ;-()) before passed to PR_SetEnv() - and the same applies to
PS_GetEnv() - the string obtained from that function call needs to be converted
from the current locale ($LANG) to Unicode.

Comment 4

[Simon Montagu :smontagu]

>Env var strings are encoded in the current locale and it is valid to have chars
>outside the ASCII range (so PromiseFlagCString isn't the correct way... jshin -
>can you help ?)

A CString can be in any encoding, and isn't limited to the ASCII range.

Comment 5

[Roland Mainz]

Simon Montagu wrote:
> > Env var strings are encoded in the current locale and it is valid to have 
> > chars outside the ASCII range (so PromiseFlagCString isn't the correct 
> > way... jshin - can you help ?)
>
> A CString can be in any encoding, and isn't limited to the ASCII range.

OKOK, point for you... :))

... but I do not see where the nsIEnvironment code does the conversion between
JS unicode and local encoding... correct me if I am wrong - the code only
accepts/returns ASCII input/output for variable names and variable values, right
?

Comment 6

[Jungshik Shin]

+    ACString get(in ACString aName);
+    void set(in ACString aName, in ACString aValue);

I think the return value of getter and |aValue| of setter should be AUTF8String
or AString. Otherwise, they wouldn't be preserved across the XPconnect boundary.
In addition, we need to do the conversion to/from the locale encoding as Roland
pointed out.

Comment 7

[Roland Mainz]

jshin: See bug 171809 for the conversion function...

Comment 8

[Benjamin Smedberg]

> 1. An enumerator to list all current environment variables.
> 2. There should be an observer mechanism that the code within Mozills can
> track env variable changes.

I agree this would be nice, but NSPR doesn't provide any way to do it.

As for the native encoding issues, how does this sound: (modeled after nsIFile)

+    /**
+     * Get the value of an environment variable. The system charset is used.
+     * If the variable has not been set, the value returned is the empty string.
+     */
+    AString get(in AString aName);
+    [noscript] ACString getNative(in ACString aName);
+
+    /**
+     * Set the value of an environment variable. The system charset is used.
+     * If the value is the empty string, the variable may be unset or may
+     * be set to the empty string, depending on the platform.
+     */
+    void set(in AString aName, in AString aValue);
+    [noscript] void setNative(in ACString aName, in ACString aValue);

Comment 9

[timeless]

Comment on attachment 136819 [details] [diff] [review]
new interface nsIEnvironment

>+        var environment = Components.classes["@mozilla.org/process/environment;1"].
>+            createInstance(Components.interfaces.nsIEnvironment);
finally found a bug :). you wanted this thing to be a service, so that should
be getService :)

Comment 10

[Roland Mainz]

Benjamin Smedberg wrote:
> > 1. An enumerator to list all current environment variables.
> > 2. There should be an observer mechanism that the code within Mozills can
> > track env variable changes.
>
> I agree this would be nice, but NSPR doesn't provide any way to do it.

[2] can easily be implemennted - assuming that the observer only gets called if
the env vars are changed using the nsIEnvironment API.
[1] may require to save the env pointer which is usually passed as 3rd argument
to |main| (e.g. |int main(int ac, char *av[], char *env[]|)

> As for the native encoding issues, how does this sound: (modeled after 
> nsIFile)
> 
> +    /**
> +     * Get the value of an environment variable. The system charset is used.
> +     * If the variable has not been set, the value returned is the empty > 
> string.
> +     */
> +    AString get(in AString aName);
> +    [noscript] ACString getNative(in ACString aName);
> +
> +    /**
> +     * Set the value of an environment variable. The system charset is used.
> +     * If the value is the empty string, the variable may be unset or may
> +     * be set to the empty string, depending on the platform.
> +     */
> +    void set(in AString aName, in AString aValue);
> +    [noscript] void setNative(in ACString aName, in ACString aValue);

Whhhhhhaaaaaa. No, please no. Another API which calls for trouble in the future
(and if nsIFile has such an API it should be removed unless there is a VERY good
reason why it is required).
Access to environment variables should always go througth the platform charset
converters for both |set| and |get| to ensure that proper conversion is done.

Comment 11

[Jungshik Shin]

> Access to environment variables should always go througth the platform charset
> converters for both |set| and |get|

 Having both native and Unicode APIs is orthogonal to the above (, to which I
guess everybody certainly agrees), isn't it? 

> Another API which calls for trouble in the future
    
   Are you worried about potential misuses or the redundancy? As for the latter,
noscript 'native' version would be justified if there's a good use case. As for
the former, it seems to be pretty clear which is for what from their names. 

Comment 12

[Roland Mainz]

Jungshik Shin wrote:
> > Access to environment variables should always go througth the platform 
> > charset
> > converters for both |set| and |get|
>
> Having both native and Unicode APIs is orthogonal to the above (, to which I
> guess everybody certainly agrees), isn't it? 

The version of the function calls which doesn't do the conversion is both
redundant and risky - see below.

> > Another API which calls for trouble in the future
> 
> Are you worried about potential misuses or the redundancy? As for the latter,
> noscript 'native' version would be justified if there's a good use case. As 
> for the former,
> it seems to be pretty clear which is for what from their names. 

I am worried about both problems. If the values has non-ASCII chars a conversion
MUST be done (otherwise we're in trouble and have to deal with another bug
report in bugzilla), so having a 2nd set of function calls which don't do a
conversion is _redundant_.
And having such an API is calling for trouble since people usually try stuff
like |NS_ConvertUCS2toUTF8| or |NS_ConvertUCS2toASCII| without thinking about
the consequences (please don't try to come up with the argument "review will
catch that"). The printing code is the best example where the misdesign of the
API has lead to a problem which hasn't been solved even after three years
(remember the bunch of bugs in bugzilla about that issue ?).

The only argumentation for a "native" version of the nsIEnvironemnt API which
bypasses the platform encoding converters would be performace - but
setting/getting env vars is NOT performace critical (just look at the way how  
|putenv()| is implemented in Unix... and scream).

Comment 13

[Jungshik Shin]

In the first place, I didn't think there's any strong need for the noscript
version. I thought you'd have some use cases, but you don't. Then, we can just
go with the unicode version which wraps up the encoding conversion in the
implementation.
 

Comment 14

[Benjamin Smedberg]

Please take a look at the current users of PR_SetEnv. Most of them have no use
for character-conversion, as the most they do is set a variable name =1 or =some
integer or well-known file path.  Requiring them to do a charset conversion to
use this interface is overkill and silly. It's only when you have "unknown" data
(font names passed in from elsewhere, file paths with potential non-ASCII chars)
that we even need to bother with charset conversion.

Comment 15

[Benjamin Smedberg]

Attachment: patch, take 2

Comment 16

[Benjamin Smedberg]

Attachment: nsIEnvironment, take 2.1

Maybe I should compile and test patches before posting them.

Comment 17

[Roland Mainz]

Comment on attachment 137712 [details] [diff] [review]
nsIEnvironment, take 2.1

Benjamin Smedberg wrote:
> Please take a look at the current users of PR_SetEnv. Most of them have no use
> for character-conversion, as the most they do is set a variable name =1 or 
> =some integer or well-known file path.  Requiring them to do a charset 
> conversion to use this interface is overkill and silly.

This is not silly. Main consumer of nsIEnvironment are scripts which MUST go
througth the conversion.
Same applies to normal C/C++ code where the usage of conversion should be
_enforced_ to _gurantee_ that the correct conversion is being done. Your patch
allowes code to bypass this - which caused lots of trouble in the past.

Please remove the SetNative()/GetNative() functions from the patch.

Comment 18

[Jungshik Shin]

I'm not strongly against having native APIs (with the name 'Native', it appears
pretty clear what they are). However, your justification for them doesn't seem
to hold up.

> current users of PR_SetEnv. Most of them have no use
> for character-conversion, as the most they do is set a variable name =1 or 
> =some integer or well-known file path.

'Well-known file paths' are NOT always ASCII. They (profile directory, gre
directory, and so path) have been the source of the problem because they're
assumed to be ASCII-only. 
  
Lxr (on Mozilla) gave me only   7 hits outside NSPR and only one of them is
setting the value to an integer. Others involve 'file path' that should go
through the conversion. Currently, they don't and I guess changing them to use
nsIEnvironment would fix several outstanding bugs. 

Anyway, an alternative way to deal with all ASCII cases is to provide Unicode
APIs only in which IsASCII() is called to check if the conversion is necessary.

Comment 19

[Roland Mainz]

Jungshik Shin wrote:
> I'm not strongly against having native APIs (with the name 'Native', it 
> appears pretty clear what they are).

We have other stuff in the tree which is clearly marked as "only use with ASCII
input". The problem is that this kind of additional ASCII-only API was misused
in the past, causing lots of "fun" (usually the problems start like this: people
usually hack the code until the input datatype is correct and the compiler stops
complaining about syntax errors - and then the code gets tested with normal
english input... and a few months later we have another bug about something
which doesn't work with non-ISO-Latin-1 input ... ;-((( ).
I simply don't want to see more of these bugs anymore.

Simple rule:
Anything within Mozilla should use unicode with no exception (unless there is a
very very good reason to break the rule - and in that case i18n experts like
smontagu, jshin, katakai, prabhat etc. should do the decision (sort of
three-fourths majority like in the parliament :) whether it is allowed to break
it).
Conversion between unicode and the current platform charset should be done (or
better: enforced) by the functions which wrap native OS functions to gurantee
that everything works OK.

[snip]
> > current users of PR_SetEnv. Most of them have no use
> > for character-conversion, as the most they do is set a variable name =1 or 
> > =some integer or well-known file path.
>
> 'Well-known file paths' are NOT always ASCII. They (profile directory, gre
> directory, and so path) have been the source of the problem because they're
> assumed to be ASCII-only. 

Even source code can have chars outside the ASCII range (think about
ISO8859-1/-15 where באצה�� etc. are valid chars in the filesystem and env
variables (both variable name and value!)) - and most C/C++/Fortran compilers
even allow multitype chars within the source code (including string literals).

> Lxr (on Mozilla) gave me only   7 hits outside NSPR and only one of them is
> setting the value to an integer. Others involve 'file path' that should go
> through the conversion. Currently, they don't and I guess changing them to use
> nsIEnvironment would fix several outstanding bugs. 
>
> Anyway, an alternative way to deal with all ASCII cases is to provide Unicode
> APIs only in which IsASCII() is called to check if the conversion is 
> necessary.

That's may be a good solution... :)

Comment 20

[Roland Mainz]

BTW: The patch looks fine (except the |GetNative()|/|SetNative()| horror) ... :)

Two nits:
- There is no code for ther observer stuff yet... if you don't implement it
please add a comment to the IDL that it should not be frozen before the API gets
completed.
- A nice feature would be to have NSPR logging which tracks the _native_ strings
passed to/received from PR_GetEnv()/PR_SetEnv(), conversion failure (!!) and
observer calls.

Comment 21

[Jungshik Shin]

There's a major problem here. On Win 2k/XP, by using PR_GetEnv(), we're
restricted to the character repertoire of the current system codepage although
Win 2k/XP 'environment variables' can contain any Unicode character. We need
PR_GetEnv_UTF16() or sth. like that (see bug 162361). Alternatively, we have to
switch to UTF-8 codepage on Win2k/XP (as soon as Mozilla starts) as darin once
suggested.
 

Comment 22

[Jungshik Shin]

Unlike some other 'W' APIs on Win32, '_wgetenv' and '_wputenv' appear to be
available across Win32 platforms (Win9x through WinXP) regardless of the
presence of MSLU (Microsoft Layer for Unicode). [1] That means, we don't have to
check the OS (Win9x/ME vs Win 2k/XP) and we can just call UTF16 version (after
adding them to NSPR). 

#ifdef XP_WIN
call PR_GetEnv_UTF16
#else
call PR_GetEnv after converting to native charset
#endif

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/html/_crt_getenv.2c_._wgetenv.asp

Comment 23

[Benjamin Smedberg]

Attachment: nsIEnvironment, take 3

ok, finally understand the problem after discussion with biesi: not all native
charsets are supersets of ASCII, so even native ASCII usage is fundamentally
flawed. I'm kinda unhappy that the NS_Copy*To* functions don't throw any
errors, ever... LOSS_OF_SIGNIFICANT_DATA errors would be welcome.

This interface is *not* going to do environment enumeration, because that
cannot be accomplished XP. We can do a new interface nsIEnvironment2 if that
becomes necessary and available in the future. An observer system doesn't need
to be part of the interface, it can be part of the global nsIObserver service
if someone decides to implement it.

Comment 24

[Jungshik Shin]

> not all native charsets are supersets of ASCII, 

  That's true but only on yet-to-be-materialized hypothetical (at least to me
although not to someone working on them) systems. On Win2k/XP, arguably, it's
UTF-16LE so that we have such a system right now, but PR_GetEnv (and other parts
of NSPR) doesn't (don't)  support it yet. 

> so even native ASCII usage is fundamentally flawed. 

  Still, it's a good idea to get rid of native APIs from nsIEnvironment design
point of view.

> I'm kinda unhappy that the NS_Copy*To* functions don't throw any
> errors, ever... LOSS_OF_SIGNIFICANT_DATA errors would be welcome.

   
See comment #22 and the reference to MSDN there. It has to be specialized for
Win32. That is, you ave to use  OS APIs (_wgetenv and _wputenv) directrly on
Win32 (with #ifdef XP_WIN). NS_NativeToUnicode/NS_UnicodeToNative can't help you
on Win32 at the moment.

Comment 25

[Benjamin Smedberg]

> See comment #22 and the reference to MSDN there. It has to be specialized for
> Win32. That is, you ave to use  OS APIs (_wgetenv and _wputenv) directrly on

What I've got here is no worse than PR_SetEnv we currently use, so I'd like to
go ahead and get the interface (which is correct) into the tree to help with
some xpinstall stuff. As a later step we can either implement and use PR_SetEnvW
(which is much preferable) or use the win32 apis directly. Be aware that win32
processes keep *three* separate environments:

 1. C-runtime/posix in MBCS (getenv,putenv)
 2. C-runtime/posix in Unicode (wgetenv,wputenv)
 3. win32 environment in MBCS on 9x and unicode on NT (SetEnvironmentVariable
and GetEnvironmentVariable)

1 & 2 are linked together by the C runtime to keep them in sync (which
occasionally fails when the unicode->MBCS mapping is ambiguous).

3 is entirely separate (if you putenv, you won't see it in GetEnvironmentVariable)

This is discussed in detail in bug 66613. We are ambiguous about *which*
environment child processes inherit.

Basically, we need to fix NSPR, not muck around with platform-specific code here.

Comment 26

[Jungshik Shin]

OK. That's fair enough. Thanks for the pointer to bug 66613

[off-topic] Who's gonna move forward this and other necessary changes (like
Unicode file I/O) for the full Unicode support on Win2k/XP in NSPR? It's been
frustrating. See how old bug 66613 is (and bug 162361).

Comment 27

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

Comment on attachment 138048 [details] [diff] [review]
nsIEnvironment, take 3

Clients should treat the empty string as
+     * 	       equivalent to an unset variable.

hm. so I can't find out whether the variable is set, but empty?

Comment 28

[Benjamin Smedberg]

> hm. so I can't find out whether the variable is set, but empty?

Correct... according to NSPR they should be treated equivalently, because there
is no XP way to unset an environment var.
http://lxr.mozilla.org/mozilla/source/nsprpub/pr/include/prenv.h#80

Comment 29

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

you'd think that such useful things were documented in the nspr reference, but no...
http://www.mozilla.org/projects/nspr/reference/html/prsystem.html#25208 doesn't
mention that

Comment 30

[Roland Mainz]

Comment on attachment 138048 [details] [diff] [review]
nsIEnvironment, take 3

+    nsCAutoString nativeVal;
+    char *value = PR_GetEnv(nativeName.get());
+    if (value) {
+	 rv = NS_CopyNativeToUnicode(nsDependentCString(value), aOutValue);
+    } else {
+	 aOutValue.Truncate();
+	 rv = NS_OK;
+    }

Uhm, how should a JavaScript application check whether a env variable is
actually set or just contains an empty string ?

IMO an error should be returned... possible canidates are
|NS_ERROR_NOT_AVAILABLE| or |NS_ERROR_FILE_NOT_FOUND|. Or define a set of new
return codes (like |NS_ERROR_NO_SUCH_ENVIRONMENT_VARIABLE|) ...

Comment 31

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

gisburn: did you read comment 28?

Comment 32

[Roland Mainz]

Christian Biesinger wrote:
> gisburn: did you read comment 28?

Erm... somehow my brain skipped that part while catching-up with the latest bug
updates... sorry...

... but I disagree with the idea that an _empty_ env var _value_ is the same as
_no_ _env_ _variable_ set.
AFAIK at least on Unix/Linux there is a difference - |getenv| only returns an
pointer to the value if the env variable is present, otherwise |NULL| is
returned.
IMHO on Unix/Linux we should behave in the same way since Unix/Linux
applications make a difference between "no var set" and "var has empty value".
Otherwise we can't handle this difference from JavaScript-based Mozilla
applications (and maybe we later have a bug report just to get that problem
fixed because someone needs it).

BTW: PR_GetEnv() is just a wrapper function for |getenv()| on Unix/Linux so the
same rules should apply (othwise the behaviour of |PR_GetEnv()| should be
changed to conform to the behaviour described in the NSPR documentation).

Comment 33

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

that's nice, but what if you want to programmatically clear an environment
variable? you can't (with NSPR). all you can do is PR_SetEnv("FOO="), and that
gives you the empty string.

Comment 34

[Roland Mainz]

Christian Biesinger wrote:
> that's nice, but what if you want to programmatically clear an environment
> variable? you can't (with NSPR).

I know that the Unix API is asymmetric in this case - but that doesn't mean we
have to restrict the nsIEnvironment API to a common subset. Sooner or later such
a design decision will bite back...

... what about adding a simple method |exists()| to the nsIEnvironment API ?
then |get()| doesn't need to be changed and there is an optional way to probe
whether an env var is actually be set or not.

Comment 35

[Benjamin Smedberg]

We should not have an .exists() method, because it's promising information that
we can't deliver. Authors should treat the empty variable the same as the
missing variable. Under what situations would you need to distinguish the
non-set variable from the empty variable?

Comment 36

[Roland Mainz]

Benjamin Smedberg wrote:
> We should not have an .exists() method, because it's promising information 
> that we can't deliver.

Huh ?
What do you think 
env_var_exists = (getenv(name) != NULL);

> Authors should treat the empty variable the same as the
> missing variable.

Wrong. This are completely different things. At least Unix makes that differece.
Take a look at the sh(1) manual page and the CDE specs - they make a clear
seperation between "env var exists" and "env var has empty value". The bourne
shell (e.g. /bin/sh) even provides a way to undefine env vars (note that the
bourne shell (I don't mean bash) is very simplistic and does not have much
redundant stuff. If |x=""| would sufficient the shell would not provide an extra
|unset|). Guess why ?

> Under what situations would you need to distinguish the
> non-set variable from the empty variable?

If the env var was set by someone the empty value has a meaning.
if the env var was not set there is no meaning and we can set our own one
without worring to override user settings.

Comment 37

[Darin Fisher]

roland: we cannot deliver information that NSPR does not provide.  NSPR makes it
very clear that "exists" cannot be implemented cross-platform.  we could maybe
provide nsIEnvironmentUnix or something like that if we really wanted to expose
platform specific details.  but without a compelling use-case, what's the point?

Comment 38

[Roland Mainz]

Darin Fisher wrote:
> roland: we cannot deliver information that NSPR does not provide.  NSPR makes 
> it very clear that "exists" cannot be implemented cross-platform. 

Then I suggest to change that comment in NSPR instead of violating standard Unix
and POSIX behaviour.

> we could maybe
> provide nsIEnvironmentUnix or something like that if we really wanted to 
> expose platform specific details.  but without a compelling use-case, what's 
> the point?

There is a simple one...  bsmedberg filed this bug AFAIK to get some progress
for the Xprt XPI (that's bug 188733) so we can use that as example:
- If the env var XPSERVERLIST does not exist we're free to set it ourselves with
any values we want (even start our own Mozilla-speicifc Xprint server)
- If the env var XPSERVERLIST _exists_ with an _empty_ value then someone set
this env var explicitly to an empty value to disable the Xprint support

Comment 39

[Darin Fisher]

roland:

ok, so it sounds like we need to special case the implementation of
nsIEnvironment to use |getenv| directly on platforms where it is supported.  we
would need to come up with a reasonable implementation for Exists for other
platforms where there is no way to discern an empty env var from a non-existant one.

Comment 40

[Roland Mainz]

Darin Fisher wrote:
> ok, so it sounds like we need to special case the implementation of
> nsIEnvironment to use |getenv| directly on platforms where it is supported.

Uhm... really ? Currently |PR_GetEnv()| is a wrapper for |getenv()| on Unix.

> we would need to come up with a reasonable implementation for Exists for other
> platforms where there is no way to discern an empty env var from a 
> non-existant one.

... or just ignore the problem for non-Unix platforms... if I understand it
correctly (correct me if I am wrong :) |PR_GetEnv()| will return always an empty
string ("") on platforms which do not support the Unix behaviour -
|nsIEnvironment.exists()| would then always return |True|. Is that acceptable or
do you think it's mandatory to make a |nsIUnixEnvironment|-subclass of
|nsIEnvironment| just to add that single method ?

Comment 41

[Benjamin Smedberg]

The problem is not PR_GetEnv, which works correctly on all platforms (returns
nsnull for not-set, "" for empty).

The problem is with some unix-y platforms (AIX perhaps?): setenv cannot reliable
un-set variables. Therefore the NSPR programming guidelines say that we must
treat null and "" equivalently.

I looked for xprint documentation, and nowhere does it say that an empty
XPSERVERLIST disables xprint. http://www.lesstif.org/XpREADME.html merely states
that the var is a search path... other xprint servers could be entered manually.

If a missing XPSERVERLIST means something different from XPSERVERLIST=""
(something I strongly discourage), we need to make it clear that the exists()
method is a crutch method, and there is no reliable way to un-set a variable
from mozilla.

Comment 42

[Darin Fisher]

bsmedberg: thanks for clarification on the NSPR docs... so, hmm... i guess it
all just comes down to whether or not we really need to be able to discern an
empty environment variable from a non-existant one.  you and roland don't seem
to agree on that bit ;-)

i'm fine with exposing an exists function if it is needed, but i agree that it
should be left off if it is not needed.

Comment 43

[Roland Mainz]

Benjamin Smedberg wrote:
> The problem is not PR_GetEnv, which works correctly on all platforms (returns
> nsnull for not-set, "" for empty).
>
> The problem is with some unix-y platforms (AIX perhaps?): setenv cannot 
> reliable un-set variables. Therefore the NSPR programming guidelines say that 
> we must treat null and "" equivalently.

I am not talking about |setenv()| or unsetting env vars from within mozilla. I
am talking about a way to check whether an env var is _present_ or
_not_present_, likely some caller of mozilla/GRE (like a shell script) set that
env var. I am looking for a way to get an information ("does exist" or "does not
exist" via |nsIEnvironment.exists()|), regardless whether I can modify the
results with some other method (e.g. something like |nsIEnvironment.unset()|).

> I looked for xprint documentation, and nowhere does it say that an empty
> XPSERVERLIST disables xprint.

I am following the way how the CDEnext code is implemented (XPSERVERLIST env var
(if present) overrides builtin defaults, the XpServerList Xrm application
resource (if present) overrides the XPSERVERLIST env var etc.).

> http://www.lesstif.org/XpREADME.html merely 
> states that the var is a search path... other xprint servers could be entered
> manually.

Uhm... http://www.lesstif.org/XpREADME.html is the same as
xc/programs/Xserver/XpConfig/README which is very incomplete (for example it
does not cover the behaviour of the XPRINTER/LPDEST/etc. env vars nor the Xrm
application resource equivalents for the XPSERVERLIST/XPRINTER/etc. env vars
(BTW: support for that is missing in XprintUtils)).

> If a missing XPSERVERLIST means something different from XPSERVERLIST=""
> (something I strongly discourage),

Why should this be discouraged ? This is a normal behaviour for Unix. Similar
behaviour can be AFAIK found in Motif and elsewhere (see my example about the
bourne shell above).

> we need to make it clear that the exists()
> method is a crutch method, and there is no reliable way to un-set a variable
> from mozilla.

... I never asked for a |nsIEnvironment.unset()| (or did I ? I hope not... :)
... just for a |nsIEnvironment.exists()| ...

Comment 44

[Roland Mainz]

bsmedberg:
I like your yesterday's idea on IRC #mozilla about adding |exists()| and simply
add a huge scary comment... I guess everyone will be hapy with that... :) 

Comment 45

[Roland Mainz]

Taking myself per IRC discussion with bsmedberg (I want this "in" as part of
moz1.7a) ...

Comment 46

[Roland Mainz]

Attachment: Patch for 2004-01-14-trunk

Comment 47

[Roland Mainz]

Comment on attachment 139086 [details] [diff] [review]
Patch for 2004-01-14-trunk

Requesting r=/sr= ...

Comment 48

[Roland Mainz]

Attachment: Updated patch, removed FORCE_LOGGING per IRC comments

Comment 49

[Roland Mainz]

Attachment: Alternative patch, NSPR logging completely removed

Comment 50

[Roland Mainz]

Attachment: New patch per dougt's IRC comments

Comment 51

[Doug Turner (:dougt)]

Comment on attachment 139092 [details] [diff] [review]
New patch per dougt's IRC comments

since this interface isn't frozen, the CID and contract id should not be listed
in nsXPCOMCID.h.

If the QI fails in the Create function	(suppose you are given a non existent
aIID),	you will leak the |obj|. 

C++, or C even, isn't very consistent with it use of trailing semicolon's, but
we should be: 

+nsEnvironment::~nsEnvironment()
+{
+    if (mLock)
+	 PR_DestroyLock(mLock);
+};  <-----

i *hate* the |Exist| method.  I do not think we need it especially in a XP
interface such as this.  however, i will let it slide and see how it is used. 
We can readdress this issue when it is time to freeze this interface.  

This interface isn't under review:
+ * @status UNDER_REVIEW

Comment 52

[Roland Mainz]

Attachment: New patch per dougt's last comments

Doug T wrote:
> (From update of attachment 139092 [details] [diff] [review])
> since this interface isn't frozen, the CID and contract id should not be 
> listed in nsXPCOMCID.h.
>
> If the QI fails in the Create function  (suppose you are given a non existent

> aIID),  you will leak the |obj|. 

Fixed.

> C++, or C even, isn't very consistent with it use of trailing semicolon's,
but
> we should be:
>
> +nsEnvironment::~nsEnvironment()
> +{
> +    if (mLock)
> +	   PR_DestroyLock(mLock);
> +};  <-----

Fixed.

> i *hate* the |Exist| method.	I do not think we need it especially in a XP
> interface such as this.  however, i will let it slide and see how it is used.


Unix/Linux differ between the cases "no env var exists" and "env var has empty
value" and we need this to allow applications to correctly use the sematics. I
added that comment to the IDL file, too.
 
> We can readdress this issue when it is time to freeze this interface.  

If we change that later we will break again stuff which will use this interface
(at that point this work would be useless then and goes straight the WONTFIX
way...).

> This interface isn't under review:
> + * @status UNDER_REVIEW

Fixed.

Comment 53

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

+    nsEnvironment() { };

you should not have a semicolon here either

Comment 54

[Roland Mainz]

Christian Biesinger wrote:
> +    nsEnvironment() { };
> you should not have a semicolon here either

OK... dougt/biesi - any more items to fix for the next patch ?

Comment 55

[Doug Turner (:dougt)]

Comment on attachment 139104 [details] [diff] [review]
New patch per dougt's last comments

drop the change to nsXPCOMCID.h.

cid's and contract id's do not go in idl files.  Put them in nsEnvironment.h. 
While you are at it, add some contributors to the license header.

again, i am very unhappy about the |exists| method.  Since there is no way to
write cross platform code with its "behaviour" undefined on some platforms, I
want you to remove it from this interface or make the other platform lie and
say that it always exists or doesn't -- that is, define this method on all
platforms.

Comment 56

[Roland Mainz]

Attachment: New patch

Doug T wrote:
> (From update of attachment 139104 [details] [diff] [review])
> drop the change to nsXPCOMCID.h.

Fixed.

... and biesi's nit is fixed, too.

> cid's and contract id's do not go in idl files.  Put them in nsEnvironment.h.

> While you are at it, add some contributors to the license header.

bsmedberg is already listed as inital developer...

> again, i am very unhappy about the |exists| method.  Since there is no way to

> write cross platform code with its "behaviour" undefined on some platforms, I

> want you to remove it from this interface

If we do that then we cannot use this interface on Unix/Linux at all since we
cannot implement default Unix behaviour (just take a look how X11 and other
Unix applications test for env vars).

> or make the other platform lie and
> say that it always exists or doesn't -- that is, define this method on all
> platforms.

I added a definition for non-Unix platforms which depends on the checking
whether the string returned by |Get()| is empty or not.
I hope that's good enougth... :)

Comment 57

[Doug Turner (:dougt)]

Comment on attachment 139176 [details] [diff] [review]
New patch

I just don't get this exist function.  If "Clients should treat the empty
string as equivalent to an unset variable", why do we need exists.

other than that, this patch is fine.

Comment 58

[Roland Mainz]

Doug T wrote:
> (From update of attachment 139176 [details] [diff] [review])
> I just don't get this exist function.  If "Clients should treat the empty
> string as equivalent to an unset variable", why do we need exists.

I guess you mean this, right ?
-- snip --
+    /**
+     * Get the value of an environment variable.
+     * @param aName   the variable name to retrieve.
+     * @return        if the variable has not been set, the value returned is 
an
+     *                empty string. Clients should treat the empty string as
+     *                equivalent to an unset variable.
+     */
+    AString get(in AString aName);
-- snip --
... I simply forgot to update the comment for |get()|, that's all.

Comment 59

[Roland Mainz]

Attachment: New patch with updated comment

Comment 61

[Doug Turner (:dougt)]

Comment on attachment 139693 [details] [diff] [review]
New patch with updated comment

good work r/sr.

Comment 62

[Roland Mainz]

Patch checked-in by Neil
(http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1074853320&maxdate=1074854160&who=neil%25parkwaycc.co.uk)
...

... marking bug as FIXED.