282809 - Path separators and objdirs incorrect in seamonkey-utils

Status: RESOLVED WONTFIX

(Webtools Graveyard :: Tinderbox, defect)

Description

[Mike Kaply [:mkaply]]

OK, this was a strange one.

We were having a build problem on OS/2 that took me forever to find out.

Basically, the code that sets LIBRARY_PATH and other env variables assumes that
the path separator is a : whereas on OS/2 it is a ; . This was causing our
environment variables to get screwed up.

In addition, I discovered places in the TB code where the indiscriminantly
insert objdir into paths without checking if objdir is set as they do in other
places.

Patch coming.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Fix for problem

Add a new variable called PathSep and use it.

Also nest some objdir adding code in an if.

Comment 2

[Chase Phillips]

Comment on attachment 174765 [details] [diff] [review]
Fix for problem

Apologies it took so long to get review on this.  The release blitz we had
snowed me under and I'm just now starting to dig my way out.

I like this patch for OS/2's sake and think it does the right thing for the
code that was in 1.296, though I find the code in 1.296 frustrating because of
how it allows for colons and semicolons to sneak into strings.	A final
solution is to treat these path variables as arrays, shift/push as needed to
add entries, then join() them using the correct separator when it's time to set
the variable's value.  Maybe we can look for that sort of fix later on down the
line.

As-is, only small nits inline.

>Index: build-seamonkey-util.pl
>===================================================================
>RCS file: /cvsroot/mozilla/tools/tinderbox/build-seamonkey-util.pl,v
>retrieving revision 1.296
>diff -u -r1.296 build-seamonkey-util.pl
>--- build-seamonkey-util.pl	10 Feb 2005 17:30:06 -0000	1.296
>+++ build-seamonkey-util.pl	19 Feb 2005 04:53:34 -0000
>@@ -376,9 +376,9 @@
>     }
> 
>     if ($Settings::ObjDir ne '') {
>-        $ENV{LD_LIBRARY_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin:" . "$ENV{LD_LIBRARY_PATH}";
>+        $ENV{LD_LIBRARY_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin$Settings::PathSep" . "$ENV{LD_LIBRARY_PATH}";

Let's simplify this code to:

  $ENV{LD_LIBRARY_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin" \
			  . $Settings::PathSep \
			  . $ENV{LD_LIBRARY_PATH};

I don't think the one line should be split across three lines using \ (unless
it's too long already).  I just placed those multiple lines here for
readability.  I do think the path separation should be made apparent by having
it be a more visible and separate part of a string-level concatenation using
the '.' operator.  Also, this example line removes the doublequotes around
$ENV{LD_LIBRARY_PATH} where they're redundant.

(I understand you were doing a simple s&r on ':' but let's use the opportunity
to straighten out rough edges we come across.)

>     } else {
>-        $ENV{LD_LIBRARY_PATH} = "$topsrcdir/$Settings::DistBin:" . ($ENV{LD_LIBRARY_PATH} || "");
>+        $ENV{LD_LIBRARY_PATH} = "$topsrcdir/$Settings::DistBin$Settings::PathSep" . ($ENV{LD_LIBRARY_PATH} || "");

I find this ($ENV{LD_LIBRARY_PATH} || "") funny in an odd way.	If there's
anything there, it will evaluate to true and then place the value of the string
in the concatenation.  If there's nothing there, it will evaluate to false and
place "" there.  Though if there's nothing in $ENV{LD_LIBRARY_PATH} and the
variable's evaluation itself was used it would place "" there, too.  In all of
these cases, the colon gets added to the string.  This is another argument for
using arrays internally then letting join() handle the magic of when to place
the separator.

>     }
> 
>     # MacOSX needs this set.
>@@ -386,15 +386,29 @@
>         $ENV{DYLD_LIBRARY_PATH} = "$ENV{LD_LIBRARY_PATH}";
>     }
> 
>-    $ENV{LIBPATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin:"
>-        . ($ENV{LIBPATH} || "");
>-    # BeOS requires that components/ be in the library search path per bug 51655
>-    $ENV{LIBRARY_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin:"
>-	. "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin/components:"
>-        . ($ENV{LIBRARY_PATH} || "");
>-    $ENV{ADDON_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin:"
>-        . ($ENV{ADDON_PATH} || "");
>-    $ENV{MOZILLA_FIVE_HOME} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin";
>+    if ($Settings::ObjDir ne '') {
>+      $ENV{LIBPATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin$Settings::PathSep"
>+          . ($ENV{LIBPATH} || "");
>+      # BeOS requires that components/ be in the library search path per bug 51655
>+      $ENV{LIBRARY_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin$Settings::PathSep"
>+          . "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin/components:"
>+          . ($ENV{LIBRARY_PATH} || "");
>+      $ENV{ADDON_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin$Settings::PathSep"
>+          . ($ENV{ADDON_PATH} || "");
>+      $ENV{MOZILLA_FIVE_HOME} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin";
>+    } else {
>+      $ENV{LIBPATH} = "$topsrcdir/$Settings::DistBin$Settings::PathSep"
>+          . ($ENV{LIBPATH} || "");
>+      # BeOS requires that components/ be in the library search path per bug 51655
>+      $ENV{LIBRARY_PATH} = "$topsrcdir/$Settings::DistBin$Settings::PathSep"
>+          . "$topsrcdir/$Settings::DistBin/components$Settings::PathSep"
>+          . ($ENV{LIBRARY_PATH} || "");
>+      $ENV{ADDON_PATH} = "$topsrcdir/$Settings::DistBin$Settings::PathSep"
>+          . ($ENV{ADDON_PATH} || "");
>+      $ENV{MOZILLA_FIVE_HOME} = "$topsrcdir/$Settings::DistBin";
>+    }
>+
>+
>     $ENV{DISPLAY} = $Settings::DisplayServer;
>     $ENV{MOZCONFIG} = "$Settings::BaseDir/$Settings::MozConfigFileName"
>         if $Settings::MozConfigFileName ne '' and -e $Settings::MozConfigFileName;
> 
>Index: tinder-defaults.pl
>===================================================================
>RCS file: /cvsroot/mozilla/tools/tinderbox/tinder-defaults.pl,v
>retrieving revision 1.75
>diff -u -r1.75 tinder-defaults.pl
>--- tinder-defaults.pl	10 Feb 2005 21:41:01 -0000	1.75
>+++ tinder-defaults.pl	19 Feb 2005 04:53:35 -0000
>@@ -22,6 +22,9 @@
> #- You'll need to change these to suit your machine's needs
> $DisplayServer = ':0.0';
> 
>+#- Change this to ; for Windows
>+$PathSep = ':';
>+

Is there a section in the scripts that sets variables depending on the
underlying operating system environment?  If so, let's set $PathSep there
instead of requiring the admin to define it separately.

> #- Default values of command-line opts
> #-
> $BuildDepend       = 1;      # Depend or Clobber

I'd like to see this patch make it in.	Let me know if you have any questions
and ping me if/when you are able to make a new version that addresses these
comments.

Comment 3

[:Gijs (he/him)]

Attachment: Attempt to fix nits [untested]

Attempt to fix the nits in the above comment. Not sure I've been succesfull, but then again, we can't ask anymore. As an aside, I found a few other stray : that I believed should also be replaced. Please correct me if I'm wrong.

Timeless said asking bryner or dbaron for review might work for tinderbox patches. Trying. :-)

The patch is of course untested because I don't have any tinderboxen myself, nor access to those other people run (assuming of course, that everybody would be fine if I take out the running machines to test patches :-) ). MKaply, are you (still) in a position to test this?

Comment 4

[:Gijs (he/him)]

Comment on attachment 220325 [details] [diff] [review]
Attempt to fix nits [untested]

*kicks bugzilla*

Timeless, Brian, not sure if these are the right email addresses. Sorry if they're not.

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 220325 [details] [diff] [review]
Attempt to fix nits [untested]

Sorry, I don't have time to review this.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 220325 [details] [diff] [review]
Attempt to fix nits [untested]

Oops, sorry, I didn't mean to clear timeless's review.

Comment 7

[Brian Ryner (not reading)]

Ugh, not my day for Bugzilla, can you re-request review from timeless?

Comment 8

[Reed Loden [:reed]]

Comment on attachment 220325 [details] [diff] [review]
Attempt to fix nits [untested]

Re-requesting review from timeless, as per bryner.

Comment 9

[timeless]

Comment on attachment 220325 [details] [diff] [review]
Attempt to fix nits [untested]

+        $ENV{LD_LIBRARY_PATH} = "$topsrcdir/${Settings::ObjDir}/$Settings::DistBin" . $Settings::PathSep . $ENV{LD_LIBRARY_PATH};
...
+        $ENV{LD_LIBRARY_PATH} = "$topsrcdir/$Settings::DistBin" . $Settings::PathSep . ($ENV{LD_LIBRARY_PATH} || "");

it occurs to me that we only sometimes use the || "" magic which seems a bit silly.

do we need to give a heads up to windows tinderbox maintainers?

or should we make this code autodetect windows and select ; on its own?

iirc $^O has the os in some way that you can ask for windows...

Comment 10

[:Gijs (he/him)]

From what I can tell this hasn't been committed yet, and apparently nobody has answered timeless' questions yet. Are there any volunteers for the latter bit? :-)
And do you want the "|| magic" everywhere, nowhere, or just as it is right now?
(Don't look at me, because I don't know. I'm simply a patch-making minion, and a busy one at that)

Comment 11

[timeless]

*shrug* the patch should probably go in and someone should patch the things i
complained about