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

Fish integration: make sure we're not in login mode before sourcing files #5802

Closed
wants to merge 6 commits into from

Conversation

ElectreAAS
Copy link
Collaborator

@ElectreAAS ElectreAAS commented Jan 23, 2024

I believe this resolves #5761.

If I understand correctly this comment by a fish maintainer, the standard shells don't read .bashrc or equivalent on login shells. However fish does, and the comment above advises to guard most of the file behind a if not status is-login (or status is-interactive, depending on context).

I apply this advice here.

@ElectreAAS
Copy link
Collaborator Author

Sorry for the messy PR, I wanted to get this one out of my system.
I don't really like discarding the errors with source X >/dev/null 2>/dev/null; or true, if there is a problem in some code called by your init script, you should be aware of it and probably fix it.
Just my opinion though, I could restore that bit

@ElectreAAS
Copy link
Collaborator Author

Note for anyone that had the issue in the first place: your terminal app (mine is gnome terminal for instance) might be set up to launch the same shell as your login shell. This is convenient when you chsh -s, because it auto updates your terminal, but it will bug out here and maybe in other places. Search in the settings for something like "start command line as login shell" and untick it.

src/state/opamEnv.ml Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member

@ElectreAAS #5841 was merged. Would you be able to rebase your work on top of master? I can do a review afterwards

@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Feb 22, 2024

I just rebased, it should be compatible now.
Upon reflexion, this PR does 4 things that are almost disjoint:

  • refactor the source function to be (100% sujective) more elegant (242bc11)
  • Make sure the file exists before sourcing it in ALL SHELLS (b27cf91)
  • Don't silence errors with > /dev/null in ALL SHELLS (f7e893a)
  • Check that we're not in login mode before sourcing in FISH ONLY (2de9dc3)

I've come to the conclusion that the last point isn't actually desired. Since fish's documentation now clearly warns about doing this, maybe we can roll with the system's assumption that the login shell does not read .bashrc & equivalents.

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reworking tests added in #5036 to be able to integrate them in that PR, like that we can see and track the changes.

src/state/opamEnv.ml Outdated Show resolved Hide resolved
src/state/opamEnv.ml Outdated Show resolved Hide resolved
src/state/opamEnv.ml Outdated Show resolved Hide resolved
src/state/opamEnv.ml Show resolved Hide resolved
@@ -23,6 +23,7 @@ users)
* Disable ACL in Cygwin internal install to avoid permission mismatch errors [#5796 @kit-ty-kate - fix #5781]
* Add `sys-pkg-manager-cmd` as an accepted field in opamrc files [#5847 @rjbou - fix #5844]
* Fix `git-location` handling in init config file [#5848 @rjbou - fix #5845]
* [BUG] Fish integration: Make sure we're not in login mode before sourcing files [#5802 @ElectreAAS - fix #5761]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog is missing entries, e.g. changes of script lines

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good now, I'll let you be the judge

@rjbou
Copy link
Collaborator

rjbou commented Feb 23, 2024

I've added an init scripts test, from #5036 (discovered while setting it up that fish on windows is broken, see #5854). I've also added for each commit the changes in the test file.

@ElectreAAS
Copy link
Collaborator Author

Closing this PR as it was split into #5864, 5865 & 5866.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple switches in PATH after logout
3 participants