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

Use the appropriate config dir for the registry #4650

Merged
merged 3 commits into from
Mar 24, 2021

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Mar 17, 2021

It was reported on IRC that Nix would create a .config dir even though XDG_CONFIG_HOME was set.

This is the only occurence of .config I could grep for that wasn't in getConfigDir().

The test is a bit... ugly... And might be missing some other actions that could create the config dir...

But otherwise, it does test this particular case. Reverting the fix (tip of this branch) makes it fail as expected.

I guess we want to test other commands that can write to the config directory, are there others?

We probably also want to test the equivalent for the different caches.

@samueldr samueldr marked this pull request as ready for review March 17, 2021 23:15
@samueldr samueldr force-pushed the fix/registry-config-dir branch from 83e4ebc to 11b0e75 Compare March 17, 2021 23:15
tests/config.sh Outdated
# Test that files are loaded from XDG by default
export XDG_CONFIG_HOME=/tmp/home
export XDG_CONFIG_HOME=/tmp/confighome
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this export is now redundant considering the previous export.

The following tests, though, are not redundant. None are.

@edolstra
Copy link
Member

We shouldn't use fixed paths like /tmp/home in the tests since that's racy and breaks in multi-user environments. I see that recently got introduced in 895516c but it's a mistake.

@samueldr
Copy link
Member Author

Totally agree that it may be racy. I even changed it to reduce raciness assuming (verified through grepping) that the path is unique between all tests.

Though I see it's not foolproof. What strategy should be used to isolate each test files?

@samueldr
Copy link
Member Author

Are we blocking this on the fixed paths?

If so, I'll need a suggestion to make this go forward. I'm not sure what approach could be used in these tests.

If we're not blocking on the fixed paths, anything else needs to be changed?

@thufschmitt
Copy link
Member

What strategy should be used to isolate each test files?

I guess using $TEST_ROOT is good-enough (it's doesn't really bring much more guaranty if the test is run twice concurrently, but it's what everything else uses, so at least there's only one place to change if must be)

First, "XDG_CONFIG_HOME" shouldn't be named "home", as it may be
confusing compared with `$HOME`, which an upcoming test will be using.

Then, using a fixed location for the test is problematic. Use
`$TEST_ROOT` instead.
@samueldr samueldr force-pushed the fix/registry-config-dir branch from 11b0e75 to 66b8572 Compare March 19, 2021 19:39
@samueldr
Copy link
Member Author

samueldr commented Mar 19, 2021

Applied the fix to use $TEST_ROOT.

  • Tested the new test still fails without the fix (the last commit)
  • Tested the tests work with the fix

And how? nix-build.

@edolstra edolstra merged commit d1cb956 into NixOS:master Mar 24, 2021
@edolstra
Copy link
Member

Thanks @samueldr!

@samueldr samueldr deleted the fix/registry-config-dir branch March 24, 2021 18:38
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.

4 participants