FS#66338 - [perl] bash profile script doesn't check if $PATH variable contains perl paths

Attached to Project: Arch Linux
Opened by Jakub Nowak (MrQubo) - Tuesday, 21 April 2020, 14:51 GMT
Last edited by Eli Schwartz (eschwartz) - Monday, 12 October 2020, 13:49 GMT
Task Type Bug Report
Category Packages: Core
Status Closed
Assigned To Florian Pritz (bluewind)
Sébastien Luttringer (seblu)
Architecture All
Severity Very Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 1
Private No

Details

Description:

The `perlbin.sh` script adds three new paths to the $PATH env variable but it doesn't check if those paths aren't already there.
The `perlbin.csh` doesn't check this as well but `perlbin.fish` does.

Note that `/etc/profile` from `core/filesystem` (https://git.archlinux.org/svntogit/packages.git/tree/trunk/profile?h=packages/filesystem&id=b436e401c6b2115110b99b679d386b4da2ec192f) does this properly using `appendpath()`.


I'm using tmux and the perl paths end up in $PATH twice.
This task depends upon

Closed by  Eli Schwartz (eschwartz)
Monday, 12 October 2020, 13:49 GMT
Reason for closing:  Fixed
Additional comments about closing:  filesystem 2020.08.30-1
perl 5.32.0-2

The desired functionality is implemented.

Though objections were raised about directly sourcing /etc/profile.d/* no *rationale* has been provided for wishing to do so. It seems like this change works out fine.
Comment by Eli Schwartz (eschwartz) - Tuesday, 30 June 2020, 15:29 GMT
seblu, the filesystem package should be modified to only unset the appendpath function after profile.d scripts are sourced. This would make it easier to reuse in perlbin.sh

(Actually I modified my own /etc/profile to not unset it at all.)
Comment by Sébastien Luttringer (seblu) - Tuesday, 30 June 2020, 16:11 GMT
I am not convinced that our profile.sh should start providing APIs to the scripts it calls.

In the case of perl, this "append_path" function must also be done for the csh version of the script.
So, we need to add it to the csh package too, or have and asymmetrical implementation between csh/sh scripts.

Note: On my desktop, 3 scripts are updating the PATH and could benefit of this function.
Comment by Eli Schwartz (eschwartz) - Monday, 06 July 2020, 11:46 GMT
We currently have asymmetrical implementation between the fish version of the script, and the csh/sh versions.

I fail to see why it's bad to fix it for sh too (thus making 2 out of 3 fixed), just because it doesn't also fix csh.
Comment by Sébastien Luttringer (seblu) - Monday, 06 July 2020, 12:42 GMT
Fish loads perl PATH by sourcing /usr/share/fish/vendor_conf.d/perlbin.fish. This code is doing the job properly.
I think we should do the same in sh and csh version.

Anyway, updating /etc/profile will not be enough. Each sh scripts will need to be updated to use our internal _append_path function.
I fail to see why we should start to have API in /etc/profile when this could be fixed with few lines.
Comment by Eli Schwartz (eschwartz) - Tuesday, 07 July 2020, 00:55 GMT
Each sh script will need to be updated to EITHER use our internal _append_path function OR have its own copy of the exact same logic. No one ever suggested that an update to /etc/profile would, on its own, close this bug. So it's confusing to talk about the bug as though that were suggested...

> I fail to see why we should start to have API in /etc/profile when this could be fixed with few lines.

Because those few lines either need to be copied into every sh script that needs it, or we could have *code reuse* since we already have those lines. Why is this controversial? Either we have code reuse or we don't. Which one should it be?
Comment by Sébastien Luttringer (seblu) - Tuesday, 07 July 2020, 13:18 GMT
Do you see a controversy because I don't 100% agree with you?

Again, I prefer avoid API between /etc/profile and scripts in /etc/profile.d, while it makes small code duplication.
I'm sure you are competent enough to understand difference between internal and external API.

If Florian or others profile script maintainers think adding such API help them and is the way to go, I'd change my mind.
Comment by Sébastien Luttringer (seblu) - Friday, 21 August 2020, 08:50 GMT
Florian what do you think?
Comment by Florian Pritz (bluewind) - Sunday, 30 August 2020, 08:50 GMT
I generally dislike code duplication and I think if you provide the API the other profile.d scripts become much more readable. If everyone copies the code to their scripts, you pretty much have to read it each time since you can never know if it has just been copied, copied correctly, or if it has been modified. So yeah, I'm in favour of an API mainly for the readability.
Comment by Sébastien Luttringer (seblu) - Sunday, 30 August 2020, 20:33 GMT
Ok. filesystem-2020.08.30-1 which will land in testing soon will include an append_path function.
Comment by Jelle van der Waa (jelly) - Saturday, 12 September 2020, 14:56 GMT Comment by Eli Schwartz (eschwartz) - Sunday, 13 September 2020, 01:08 GMT
Exporting a variable in a shell script only marks the variable with the export attribute. Once that attribute is set it remains set, no matter how many times you change its content attribute; you don't need to re-export it, it will simply use the updated contents when it comes time to fork off a process with that variable in the environment.

The following is identical:

export FOO
FOO=bar

vs:

FOO=bar
export FOO
Comment by ybsar (ybsar) - Saturday, 26 September 2020, 09:34 GMT
Using filesystem 2020.09.03-1 and perl 5.32.0-3, it is no longer possible to simply source specific scripts of `/etc/profile.d/*.sh`, it now requires to define `append_path` before and to unset it after. Will that new "API" be documented? Will there be a way to source it without having to copy `/etc/profile`?
Comment by Doug Newgard (Scimmia) - Saturday, 26 September 2020, 12:19 GMT
ybsar, that has nothing to do with this ticket. DO NOT hijack tickets, and use the forums, IRC, or mailing lists for support.

Edit: may apologies, it does have to do with this ticket
Comment by Eli Schwartz (eschwartz) - Sunday, 27 September 2020, 00:46 GMT
> it is no longer possible to simply source specific scripts of `/etc/profile.d/*.sh`

This was never a formally supported use case and I'm not sure why you'd want to do that. Can you provide a compelling rationale for why this is valuable functionality, such that the change should be reverted?

...

FWIW, I do customize my own /etc/profile to not `unset -f append_path` as I consider it useful functionality to reuse in my own ~/.bash_profile
If it isn't unset, it won't prevent you from re-sourcing those files...
Comment by Quentin Barnes (qbarnes) - Sunday, 27 September 2020, 14:36 GMT
I just hit the issue reported by ybsar.

I use ksh as my login shell, and with this latest update to the perl package, I now get errors spewed at me about append_path not being found when I log in.

Files under /etc/profile.d/*.sh are supposed to be parsable by any Bourne-compatible shell.
Comment by Doug Newgard (Scimmia) - Sunday, 27 September 2020, 14:43 GMT
qbarnes, and they are, just not alone. Your issue sounds more like an unmerged .pacnew file or an old filesystem package, which is not the issue being discussed here.
Comment by Quentin Barnes (qbarnes) - Sunday, 27 September 2020, 15:28 GMT
Scimmia, you commented earlier on sept 26 on an edit update that ybsar's comment *did* have to do with this ticket. I was expanding on his comment on how it's a also a concern for me with a ksh login environment.

The only .pacnew file I have on my entire system right now is "/etc/pacman.d/mirrorlist.pacnew", so I doubt it.

However, doing some further testing, just logging in with ksh is not a specific the trigger for problem, but my problem is the same as what the other person hit with re-sourcing the files under /etc/profile.d/*.sh. I also do that with my login shell environment.

My use case is that a lot of the machines I have to log into their so broken, ancient, and/or mismanaged that my kornshell environment has to be overly defensive. I have to wipe out and restart the initialization of the environment because what gets set up by clueless admins hacking profiles and borked packages breaking so much stuff when using a shell other than bash. It's cleaner, safer and more portable to wipe out everything and redo it rather than trying work around the chaotic brokenness and mess I encounter.

Aside from the chaos problem, manually sourcing the /etc/profile.d/*.sh files is also helpful in setting up X11 and other windowing environments where you're not always going through the full login process but want to get all the env vars set up before launching utilities and the display manager starts.
Comment by Doug Newgard (Scimmia) - Sunday, 27 September 2020, 15:34 GMT
qbarnes, ksh does NOT source those files directly. No sane setup does. It sources /etc/profile, which includes the new function.

If you have rogue shell init scripts somewhere that source them directly, that's on you.
Comment by Sergey Fedorov (svfedorov) - Monday, 12 October 2020, 09:10 GMT
EDITED: wrong place to post it, terribly sorry
Comment by Doug Newgard (Scimmia) - Monday, 12 October 2020, 13:29 GMT
svfedorov, if you read the comments, you would know that this is NOT the place to post it, and that your workaround is horribly wrong. DO NOT hijack tickets, and use the forums, IRC, or mailing lists if you need support.

Loading...