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

nix-profile{,-daemon}.fish: Do not source twice, fmt #12252

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ilya-bobyr
Copy link

Motivation

nix-profile-daemon.fish and nix-profile.fish scripts perform almost identical actions.
Yet, they are written with slight differences that make it harder to see what is actually the same and what is different.

This is already discussed in #9441

I think nix-profile-daemon.fish and nix-profile.fish might eventually turn out to be identical or use a common implementation.
This change is a step is a step in this direction.

You can see the differences between nix-profile-daemon.fish and nix-profile.fish here:

https://gist.github.com/ilya-bobyr/c957a9f998b85833064b2d64b4ae4062

Context

A few bug fixes and indentation changes on both sides:

  • nix-profile-daemon.fish:

    1. When checking for repeated execution, exit before the helper function add_path is defined. Making sure it does not leak to the caller.

    2. Check that $HOME is set, as it is actually used in the configuration.

    3. Use 4 space indentation in the configuration subsection, as this is the indentation used for the rest of the file and in nix-profile.fish.

    4. Mark __ETC_PROFILE_NIX_SOURCED as --global as, by default, set creates a --local variable that might not be visible to a subsequent execution of this script in the same shell. Effectively breaking the protection.

  • nix-profile.fish:

    1. Use early exit when checking for required environment variables and double execution.
      Identical to the nix-profile-daemon.fish.

    2. Use __ETC_PROFILE_NIX_SOURCED for double execution protection, similarly to nix-profile-daemon.fish.


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Mic92
Copy link
Member

Mic92 commented Jan 15, 2025

@ilya-bobyr Thanks for you pull request. However can you make the formatting changes a separate commit? That will make it easier to review it.

@ilya-bobyr ilya-bobyr force-pushed the fish-profile-source-once-and-fmt branch from c1683a9 to ce22be7 Compare January 16, 2025 09:01
@ilya-bobyr
Copy link
Author

@ilya-bobyr Thanks for you pull request. However can you make the formatting changes a separate commit? That will make it easier to review it.

@Mic92 Thank you for looking at the change.

I've split the formatting change into a separate commit.
It does make the second commit a bit shorter.

Or do you want the formatting to be a separate PR?

P.S. I have 8 more commits on top, to remove all inconsistencies between nix-profile.fish and nix-profile-daemon.fish and to fix a few bugs.

@Mic92
Copy link
Member

Mic92 commented Jan 16, 2025

Both commits in one PR is fine.

@Mic92 Mic92 self-assigned this Jan 16, 2025
`nix-profile.fish` and part of `nix-profile-daemon.fish` use 4 space
indentation.  Which is also the indentation that the fish shell
documentation is using.

Reformatting a chunk of `nix-profile-daemon.fish` from 2 space
indentation to 4 space indentation for consistency.
In order for the script not be sourced multiple times by the same shell
instance, `__ETC_PROFILE_NIX_SOURCED` needs to be set with a `--global`
flag.

Both files are almost identical.  And style differences make it harder
to see what is actually different and keep them in sync, when it is
required.
@ilya-bobyr ilya-bobyr force-pushed the fish-profile-source-once-and-fmt branch from ce22be7 to b36637c Compare January 18, 2025 20:42
@ilya-bobyr
Copy link
Author

Rebased to fix an unrelated test failure in nix-functional-tests:flakes / flake-in-submodule.
It was caused by changes in the git+file: parsing URLs in the nix binary.

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

Successfully merging this pull request may close these issues.

2 participants