This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 544652 - speed up pathmunge inside of bashrc, profile
speed up pathmunge inside of bashrc, profile
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: setup (Show other bugs)
12
All Linux
low Severity low
: ---
: ---
Assigned To: Ondrej Vasik
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-05 16:04 EST by Andreas Pedersen
Modified: 2010-06-10 18:47 EDT (History)
7 users (show)

See Also:
Fixed In Version: setup-2.8.20-1.fc13
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-06-08 15:29:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
pathmunge() performance test script (924 bytes, text/plain)
2010-05-21 18:24 EDT, Ville Skyttä
no flags Details

  None (edit)
Description Andreas Pedersen 2009-12-05 16:04:20 EST
I have an feature idea to speed up the function pathmunge for /etc/bashrc and /etc/profile.
By using bash regular expression instead of echo and egrep.

pathmunge () {
   #if ! echo $PATH | /bin/egrep -q "(^|:)$1($|:)" ; then
   if [[ ! "$PATH" =~ "(^|:)$1(:|$)" ]];then
       if [ "$2" = "after" ];then
           PATH=$PATH:$1
       else
           PATH=$1:$PATH
       fi
    fi
}

Is there any limitation from using regular expression for this purpose?
Comment 1 Ondrej Vasik 2009-12-10 08:57:08 EST
Thanks for suggestion, looks ok. Will check some corner cases and probably add this (without commented out line) in next rawhide setup build.
Comment 2 Ville Skyttä 2009-12-16 14:59:17 EST
> [[ ! "$PATH" =~ "(^|:)$1(:|$)" ]]

This is a bashism but /etc/profile is read at least by ksh and zsh in addition to bash, so it shouldn't be done for that.  I suppose doing it in /etc/bashrc would be fine though.

While at it, could change the /etc/profile version to use "/bin/grep -qE" instead of "/bin/egrep -q"; egrep is deprecated (see man page and egrep --help).
Comment 3 Ondrej Vasik 2009-12-16 15:43:13 EST
Ah, ok ... thanks for bashism warning. I'll fix this egrep usage.
Comment 4 Ondrej Vasik 2009-12-17 05:00:04 EST
I guess fixing in Rawhide is enough... Fixed in setup-2.8.12-1.fc13 , closing RAWHIDE.
Comment 5 Fedora Update System 2010-04-19 03:21:30 EDT
setup-2.8.9-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/setup-2.8.9-2.fc12
Comment 6 Fedora Update System 2010-05-17 14:58:10 EDT
setup-2.8.9-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 7 Watanabe, Yuki 2010-05-20 10:24:13 EDT
We can avoid Bashism by using the case command like this:

pathmunge () {
case ":${PATH}:" in
*:"$1":*)
    ;;
*)
    if [ "$2" = "after" ]; then
        PATH=$PATH:$1
    else
        PATH=$1:$PATH
    fi
esac
}

This is compatible with most shells.
Comment 8 Ondrej Vasik 2010-05-20 10:30:39 EDT
/etc/profile doesn't contain bashism - it still uses grep ... /etc/bashrc could use bashism - as it is used only by bash. Do you have some speed comparison between grep and your case?
Comment 9 Watanabe, Yuki 2010-05-20 11:31:01 EDT
The case command should run much faster than grep because it's the shell's builtin syntax while grep is an external command, which requires starting a new process to execute the command.

I'm not sure about speed comparison between the case command and the bashism regular expression, but both should run sufficiently fast since they're the shell's builtin.
Comment 10 Ville Skyttä 2010-05-21 18:23:54 EDT
Representative times on my F-13 box for the attached script:

$ time bash pathmunge.sh 1

real    0m2.809s
user    0m0.482s
sys     0m1.221s

$ time bash pathmunge.sh 2

real    0m0.175s
user    0m0.143s
sys     0m0.009s

$ time bash pathmunge.sh 3

real    0m0.172s
user    0m0.145s
sys     0m0.006s

The version 3 (from comment 7) also appears to work with bash, zsh, ksh, and dash.  And there's little if any difference between it and the "bashism" version on bash.  So I suppose switching to Yuki's version in /etc/profile would be a good thing to do.  And why not also in /etc/bashrc while at it.
Comment 11 Ville Skyttä 2010-05-21 18:24:52 EDT
Created attachment 415796 [details]
pathmunge() performance test script
Comment 12 Fedora Update System 2010-05-24 09:14:52 EDT
setup-2.8.20-1.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/setup-2.8.20-1.fc13
Comment 13 Fedora Update System 2010-05-24 09:16:03 EDT
setup-2.8.9-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/setup-2.8.9-3.fc12
Comment 14 Fedora Update System 2010-05-25 14:35:12 EDT
setup-2.8.9-3.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update setup'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/setup-2.8.9-3.fc12
Comment 15 Fedora Update System 2010-05-25 14:38:41 EDT
setup-2.8.20-1.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update setup'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/setup-2.8.20-1.fc13
Comment 16 Fedora Update System 2010-06-08 15:29:34 EDT
setup-2.8.9-3.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 17 Fedora Update System 2010-06-08 15:37:11 EDT
setup-2.8.20-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 18 John Sullivan 2010-06-09 10:13:39 EDT
A quick grep shows that a similar thing could be done for the packages krb5-devel, krb5-workstation and qt3, which drop scripts into /etc/profile.d/ which also try to add to PATH. (Between them, that's 5 calls to grep.)

No doubt there are other packages too, those are just the ones that I happen to have installed here. (I can't find a way through koji to search all packages for any file matching /etc/profile.d/*, which would help identify them.)
Comment 19 Ondrej Vasik 2010-06-09 11:56:12 EDT
Ok, adding their maintainers (or better said comaintainer in the case of qt3 ;) ) to CC to let them know...
Comment 20 Nalin Dahyabhai 2010-06-09 12:10:48 EDT
What, no love for csh users?
Comment 21 Ondrej Vasik 2010-06-09 15:52:44 EDT
(In reply to comment #20)
> What, no love for csh users?    

There is no pathmunge() function in csh.login nor csh.cshrc ... so I can't speed up it for them ...
Comment 22 John Sullivan 2010-06-10 06:53:07 EDT
It looks like the csh versions of the three package profile.d scripts I mentioned above use built-in csh regex features to modify path, so no change needed there. (Not a csh user, but it looks from the man page like csh can't do functions and aliases aren't powerful enough, so repeating the code each time is the only way.)

It also looks like no (bourne) script in my current profile.d (with whatever random selection of packages happen to be installed at the moment) makes use of the pathmunge function at all, rolling it themselves. Hence the 5 greps above.

If pathmunge is a supported facility that (bourne) profile.d scripts ought to be making use of, then the correct change would seem to be to modify such scripts to use it rather than roll their own whether or not they use grep directly or the case magic.
Comment 23 John Sullivan 2010-06-10 08:43:46 EDT
There is one way to refactor the csh path munging - place in profile.d the file pathmunge.csh-inc:

  foreach pathmunge_i (${path:q})
    if ( "$pathmunge_i" == "$1" ) then
      exit
    endif
  end
  unset pathmunge_i

  if ( "$2" == "after" ) then
    set path = ( ${path:q} ${1:q} )
  else
    set path = ( ${1:q} ${path:q} )
  endif

Then call as:

  source /etc/profile.d/pathmunge.csh-inc /usr/kerberos/bin

I would expect this to be somewhat faster than the exec method, but very slightly (but not much) slower than the existing method. Note however that it doesn't suffer from certain bugs in the way the existing krb5/qt3 usage:

1) Because it tests each component of $path separately it does not get confused by directories with embedded spaces. The existing code will not add "bar" if "/tmp/foo\ bar" is already in the path. (Nor will it add "/foo\ /bar" if both "/foo" and "/bar" are already in the path next to each other.)

2) Similarly because each component is tested as a whole, it does not get confused by substrings. The existing code fails to add "/usr/kerberos/bin" if "/usr/kerberos/bin/somethingelse" is already in the path.

3) The quote-modifiers in the path assignment means that any existing path components with embedded spaces are not mangled. The existing code will convert ( "/tmp/foo\ bar" ) into ( "/tmp/foo" "bar" ) if it modified the path.
Comment 24 Ville Skyttä 2010-06-10 18:47:58 EDT
(In reply to comment #22)
> If pathmunge is a supported facility that (bourne) profile.d scripts ought to
> be making use of, then the correct change would seem to be to modify such
> scripts to use it rather than roll their own whether or not they use grep
> directly or the case magic.    

My experience with that: I tried to do that in ccache for a while, but it was found that zsh didn't provide pathmunge for profile.d scripts.  It was then implemented/fixed, but due to zshrc being %config(noreplace) pathmunge might still not be available after upgrades, so I reverted to not using it and chose to use the "case magic" approach (which isn't that bad IMO) and have no plans to go back to pathmunge at least in a while, see bug 548960 for more info.  I wouldn't be surprised if there were some other shell packages with similar issues although I don't know of any at the moment.

Note You need to log in before you can comment on or make changes to this bug.