Extract prependToEnvironmentVariableList() subroutine in webkitdirs.pm.
Created attachment 266038 [details] Patch v1
Comment on attachment 266038 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=266038&action=review > Tools/ChangeLog:13 > + (appendToEnvironmentVariableList): Simplify variable name and How did you decide to change the logic of this function? I think that the new behavior is semantically wrong, at least for PATH.
(In reply to comment #2) > Comment on attachment 266038 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266038&action=review > > > Tools/ChangeLog:13 > > + (appendToEnvironmentVariableList): Simplify variable name and > > How did you decide to change the logic of this function? I think that the > new behavior is semantically wrong, at least for PATH. Because I wrote tests to check the behavior. Please describe a test case that you think is wrong.
Comment on attachment 266038 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=266038&action=review > Tools/Scripts/webkitdirs.pm:1387 > + $ENV{$name} .= ":" if substr($ENV{$name}, -1) ne ":"; This is OK as-is. I suspect that this function (both the original and after your changes) does not work when invoked from a Perl program run directly in the Windows command prompt because Windows uses ';' for its PATH separator. We should consider making use of $Config::Config{path_sep} for the platform-specific PATH separator or consider naming this function appendToUnixEnvironmentVariableList(). This work can be done in a separate patch. You can read more about $Config::Config{pah_sep} at <http://perldoc.perl.org/Config.html#p>. > Tools/Scripts/webkitdirs.pm:1399 > + my $joinCharacter = substr($ENV{$name}, 0, 1) eq ":" ? "" : ":"; Similiarly, we should use $Config::Config{path_sep} for the platform-specific PATH separator or consider naming this function prependToUnixEnvironmentVariableList(). > Tools/Scripts/webkitperl/webkitdirs_unittest/appendToEnvironmentVariableList.pl:74 > + description => "Append to single path with colon", Dyld(1) does not clarify whether DYLD_FRAMEWORK_PATH follows POSIX convention for the PATH environment variable. For the PATH environment variable, this expected result is incorrect per <http://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap08.html#tag_08_03> because a trailing colon at the end of the list of prefixes is called the zero-length prefix, which represents the current working directory. If we choose to support this legacy behavior (you at least propose supporting the zero-length prefix at the beginning of the list of prefixes) then one way to achieve conformance is to unconditionally append a colon before appending the value (as we did in the original behavior of appendToEnvironmentVariableList()). Though this can yield a PATH that has one or more zero-length prefixes (e.g. "::").
Daniel explained this in detail, a trailing colon has a different meaning (thanks Daniel!). IIRC this was discussed when the code was originally written, did you blame the code and read discussion in the bug before assuming that it was wrong?
Committed r192774: <http://trac.webkit.org/changeset/192774>
(In reply to comment #5) > Daniel explained this in detail, a trailing colon has a different meaning > (thanks Daniel!). IIRC this was discussed when the code was originally > written, did you blame the code and read discussion in the bug before > assuming that it was wrong? Nope, I didn't happen to blame it or read the discussion. Thanks for info; I made the corrections before landing the patch (no logic change to appendToEnvironmentVariableList()).