Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zsh: fix - do not source bash files and fix build #3431

Merged
merged 1 commit into from
Nov 9, 2024
Merged

Conversation

TraceyC77
Copy link
Contributor

@TraceyC77 TraceyC77 commented Jul 28, 2024

Summary

Remove sourcing of /etc/profile for zsh
Fix errors from sourcing bash scripts that are incompatible with zsh

Add patches to correct make error

Test Plan

After reboot

  • Log into TTY, verify there are no errors, ran htop
  • Launch Konsole, verify there are no errors. Verify I can call commands from solus-monorepo-helpers.zsh
  • Verified onefetch and mpv, which rely on zsh functions, work as expected
  • Verify which snap returns the binary location for snap
  • Verify /var/lib/snapd/desktop is in $XDG_DATA_DIRS once and only once
  • Installed the Spotify snap via command line. Verify it is shown in the app menu and that it launches.
  • Verify flatpaks are seen in the app menu and can be launched from there.
  • Verified $XDG_DATA_DIRS was correct
/home/$user/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share/:/usr/share/:/var/lib/snapd/desktop/:/var/lib/snapd/desktop

Checklist

  • Package was built and tested against unstable

@TraceyC77
Copy link
Contributor Author

TraceyC77 commented Jul 28, 2024

Detailed problem description:

zprofile was set to source /etc/profile - but that results errors when logging into TTY

/usr/share/defaults/etc/profile.d/50-prompt.sh:22: command not found: shopt
/home/tracey/.bashrc.d/solus-monorepo-helpers.sh:75: command not found: complete

Additionally, as shown in that output, the Solus helper script for bash was being run

The root cause was that /etc/profile, per its standard, loads all bash scripts under
/usr/share/defaults/etc/profile.d/*.sh and
/etc/profile.d/*.sh
For Solus it also loads all scripts in the user directory ~/.bashrc.d

Having zsh load /etc/profile was a customization Solus made years ago, but this is not something all other distros do. KDE Neon, for instance, does not do this, Arch does. Ubuntu based distros have a script(s) specifically to source paths necessary for snaps etc. I followed this pattern of sourcing only what's needed.

Patch file notes:

When I attempted to build the package, make encountered an error. I found patches from the openwrt file that fix it.

error:

t-frame-pointer -mno-omit-leaf-frame-pointer -Wall -Wno-error -Wp,-D_REENTRANT -fPIC -o termcap..o termcap.c  
termcap.c:45:14: error: conflicting types for ‘boolcodes’; have ‘char *[]’  
  45 | static char *boolcodes[] = {  
     |              ^~~~~~~~~  
In file included from ../../Src/zshterm.h:1,  
                from ../../Src/zsh_system.h:932,  
                from ../../Src/zsh.mdh:17,  
                from termcap.mdh:17,  
                from termcap.c:38:  
/usr/include/term.h:783:56: note: previous declaration of ‘boolcodes’ with type ‘const char * const[]’  
 783 | extern NCURSES_EXPORT_VAR(NCURSES_CONST char * const ) boolcodes[];  
     |                                                        ^~~~~~~~~  
make[3]: *** [Makefile:240: termcap..o] Error 1 

@HarveyDevel
Copy link
Member

I moved to fish awhile back so I had not noticed any issues here are some things to consider.

The shopt issue was fixed by me 4 years ago with this

if [ $SHELL != "/bin/zsh" ]; then

It has broken again with the usr-merge changes since zsh is now in /usr/bin/zsh so we could just fix the logic.

If not and zprofile stops sourcing /etc/profile then 10-path.sh is not sourced so ~/.local/bin/ will never be added to path when that dir exist as that is not being set in systemd. https://github.com/getsolus/packages/blob/main/packages/s/systemd/package.yml#L138

Having zprofile source /etc/profile unless /etc/zprofile || /etc/zsh/zprofile is added by the user was part of my fix for snaps not working for zsh users. So they should be tested too if we change this behaviour. https://phab.getsol.us/R3329:c609aba3edf7df0622c1853272260f1528c7ef82

@TraceyC77
Copy link
Contributor Author

Thanks for the insight.

Since the scope of scripts being pulled in was expanded with the inclusion of ~/.bashrc.d I'd like to see if we can do something that only satisfies pathing and makes snaps work with zsh rather than pull all of what /etc/profile pulls in. I'll do some investigation.

@TraceyC77 TraceyC77 self-assigned this Jul 29, 2024
@TraceyC77
Copy link
Contributor Author

After looking at how Ubuntu distros handle this, I have found a solution that I think will work.

Our file profile.d/70-snapd.sh does what /etc/profile.d/apps-bin-path.sh in ubuntu distros do to ensure the snap paths are included in the environment

I'm testing with a snap install of Spotify

@TraceyC77 TraceyC77 marked this pull request as ready for review August 5, 2024 03:01
@TraceyC77 TraceyC77 requested a review from ReillyBrogan August 5, 2024 03:01
@ReillyBrogan
Copy link
Contributor

Commits need squashin'

@ermo ermo added Topic: Platform Integration Integration of various components within Solus Topic: Plumbing Core components Type: Feature Something can be enhanced. labels Oct 20, 2024
@ermo ermo added this to the Solus 4.7 milestone Oct 20, 2024
Remove sourcing of /etc/profile for zsh
Fix errors from bash scripts that are incompatible with zsh
and sourcing bash scripts the user did not intend for zsh
@TraceyC77
Copy link
Contributor Author

Commits need squashin'

Done. I'd like to get this in for Hacktober if possible :)

@TraceyC77 TraceyC77 added hacktoberfest-accepted This PR is accepted for Hacktoberfest Bug Something isn't working and removed Type: Feature Something can be enhanced. labels Oct 24, 2024
@EbonJaeger
Copy link
Member

Is my understanding correct that we cannot "simply" fix the issue be fixing the check in the bash package because all the other bash scripts get unconditionally sourced, hence having to do all of this inzprofile?

Do we still need to do anything with that broken check in the bash package, either fixing the path, or removing it?

@TraceyC77 TraceyC77 mentioned this pull request Nov 1, 2024
1 task
@TraceyC77
Copy link
Contributor Author

Is my understanding correct that we cannot "simply" fix the issue be fixing the check in the bash package because all the other bash scripts get unconditionally sourced, hence having to do all of this inzprofile?

Correct.

Do we still need to do anything with that broken check in the bash package, either fixing the path, or removing it?

The if test should be removed. I opened an issue for it /issues/4214
We also have #3096 for the path

@TraceyC77
Copy link
Contributor Author

@ReillyBrogan You had said you were going to review this PR, just checking if you had the time yet. Thanks!

@TraceyC77 TraceyC77 merged commit e5fe4cf into main Nov 9, 2024
1 check passed
@TraceyC77 TraceyC77 deleted the zsh_profile_fix branch November 9, 2024 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working hacktoberfest-accepted This PR is accepted for Hacktoberfest Topic: Platform Integration Integration of various components within Solus Topic: Plumbing Core components
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants