Skip to content

Fix logic for default XDG_DATA_DIRS value#9312

Merged
fricklerhandwerk merged 1 commit intoNixOS:masterfrom
iFreilicht:fix-xdg-data-dirs-population
Nov 7, 2023
Merged

Fix logic for default XDG_DATA_DIRS value#9312
fricklerhandwerk merged 1 commit intoNixOS:masterfrom
iFreilicht:fix-xdg-data-dirs-population

Conversation

@iFreilicht
Copy link
Copy Markdown
Contributor

@iFreilicht iFreilicht commented Nov 6, 2023

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.

@iFreilicht
Fix logic for default XDG_DATA_DIRS value
a9d1963 
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 iFreilicht requested a review from edolstra as a code owner November 6, 2023 22:29
@iFreilicht
Copy link
Copy Markdown
Contributor Author

iFreilicht commented Nov 6, 2023 •
edited
Loading

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
Copy Markdown
Contributor

Hoverbear commented Nov 6, 2023 •
edited
Loading

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
Copy Markdown
Member

Ericson2314 commented Nov 6, 2023

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?

Hoverbear
Hoverbear approved these changes Nov 6, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Hoverbear commented Nov 6, 2023

A test could be added somewhere like

nix/tests/installer/default.nix

Lines 210 to 219 in fe62cb7

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
Copy Markdown

workingjubilee commented Nov 6, 2023

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

@fricklerhandwerk
Copy link
Copy Markdown
Contributor

fricklerhandwerk commented Nov 7, 2023

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

@Hoverbear please do!

@fricklerhandwerk fricklerhandwerk merged commit 1362a0a into NixOS:master Nov 7, 2023
@Hoverbear Hoverbear mentioned this pull request Nov 7, 2023
Open
@iFreilicht iFreilicht deleted the fix-xdg-data-dirs-population branch November 7, 2023 08:09
@NedAnd1 NedAnd1 mentioned this pull request Nov 20, 2023
Closed
3 tasks
@iFreilicht iFreilicht mentioned this pull request Nov 21, 2023
Merged
This was referenced Nov 22, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

1 more reviewer

@Hoverbear Hoverbear Hoverbear approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iFreilicht @Hoverbear @Ericson2314 @workingjubilee @fricklerhandwerk