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

Fix logic for default XDG_DATA_DIRS value #9312

Merged

Conversation

iFreilicht
Copy link
Contributor

Motivation

Fixes an oversight in #8985.

Context

The POSIX test manpage as well as the fish test manpage specify that -z will be "True if the length of string string is zero; otherwise, false."

The -n was likely a mixup and not caught during testing of #8985 due to a lack of missing conflicting entries in XDG_DATA_DIRS.

Priorities

Add 👍 to pull requests you find important.

The [POSIX test manpage](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html)
as well as the [fish test manpage](https://fishshell.com/docs/current/cmds/test.html#operators-for-text-strings)
specify that `-z` will be "True if the length of string string is zero;
otherwise, false."

The `-n` was likely a mixup and not caught during testing of
NixOS#8985 due to a lack of missing
conflicting entries in `XDG_DATA_DIRS`.
@iFreilicht
Copy link
Contributor Author

iFreilicht commented Nov 6, 2023

I already tested this change and can confirm that now, after installing the version of Nix from this branch and starting a new terminal session, XDG_DATA_DIRS is set correctly:

$ echo $XDG_DATA_DIRS
/home/felix/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share:/usr/share:/var/lib/snapd/desktop:/home/felix/.nix-profile/share:/nix/var/nix/profiles/default/share

@Hoverbear can you confirm that after applying the same testing from your original PR, you get the expected results?

@Hoverbear
Copy link
Contributor

Hoverbear commented Nov 6, 2023

Oh no. 😓 It seems I wasn't thinking and copied from the NIX_SSL_CERT_FILE lines below. (#8985 (comment))

This makes logical sense and I can confirm on my test VM this resolves the issue described.

image

Here is the result from #8985:

image

@Ericson2314
Copy link
Member

I don't really know how the installer tests work, but it would be nice to test this. Can one of you two take a stab at that?

Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

I believe this fixes the problem.

@Hoverbear
Copy link
Contributor

A test could be added somewhere like

source ~/.bash_profile || true
source ~/.bash_login || true
source ~/.profile || true
source /etc/bashrc || true
nix-env --version
nix --extra-experimental-features nix-command store info
out=\$(nix-build --no-substitute -E 'derivation { name = "foo"; system = "x86_64-linux"; builder = "/bin/sh"; args = ["-c" "echo foobar > \$out"]; }')
[[ \$(cat \$out) = foobar ]]
.

I believe merging this bugfix independently of an added test would cause the existing bug to impact less people.

@workingjubilee
Copy link

Maybe we can call it good if we open an issue for the regression test so that it's being tracked?

@fricklerhandwerk
Copy link
Contributor

Maybe we can call it good if we open an issue for the regression test so that it's being tracked?

@Hoverbear please do!

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

Successfully merging this pull request may close these issues.

5 participants