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

Add support for XDG base directory configuration file #2538

Merged
merged 4 commits into from May 9, 2018

Conversation

Projects
None yet
4 participants
@Mange
Contributor

Mange commented Apr 22, 2018

This is an XDG Base Directory Specification-compatible alternative to ~/.rspec.

~/.rspec has higher precedence in order to reduce risk in cases where users have a file like this dormant. It is unlikely, but this way the risk is lower.

If $XDG_CONFIG_HOME is not set, it will fall back to ~/.config, per the specification.

Name of the file

I chose the name rspec/options over, say rspec/config, because:

  • it's referred to as an "options file" in some parts of the documentation
  • it leaves space for the possibility of having a config file that is not based on CLI options (Ruby DSL, YAML, etc.)

It is trivial to change before merging, of course. I think you get to pick a name, so this is just a suggestion.

Other changes

The "isolated home" example tag has been extended with a create_fixture_file helper that also expands the filename and takes care to create the directory where the file resides in, if it does not already exist.
Other file creations have been migrated to this method for consistency.

The method is only available when an example is tagged with :isolated_home => true, for safety.

In case the developer running these tests have a custom $XDG_CONFIG_HOME set, it is cleared out when using an isolated home so their real files are not touched.

References

Add support for $XDG_CONFIG_HOME/rspec/options
This is a XDG Base Directory Specification compatible alternative to
~/.rspec.

~/.rspec has higher precedence in order to reduce risk in cases where
users have a file like this dormant. It is unlikely, but this way the
risk is lower.

If $XDG_CONFIG_HOME is not set, it will fall back to ~/.config, per the
specification.

Other changes:

The "isolated home" example tag has been extended with a
`create_fixture_file` helper that also takes care to create the
directory where the file resides in, if it does not already exist.
Other file creations have been migrated to this method for consistency.

The method is only available when an example is tagged with
`:isolated_home => true`, for safety.

In case the developer running these tests have a custom $XDG_CONFIG_HOME
set, it is cleared out when using an isolated home so their real files
are not touched.
File.join(File.expand_path(xdg_config_home), "rspec", "options")
rescue ArgumentError
# :nocov:
RSpec.warning "Unable to find $XDG_CONFIG_HOME/rspec/options because the HOME environment variable is not set"

This comment has been minimized.

@myronmarston

myronmarston Apr 23, 2018

Member

What does the HOME environment variable have to do with XDG_CONFIG_HOME?

Also, I doubt we want to warn if XDG_CONFIG_HOME is not set; my impression is that most users won't have it set (and won't care), so printing a warning will just be noisy and confusing for most users.

This comment has been minimized.

@Mange

Mange Apr 23, 2018

Contributor

It's not warning when it's not set.
This happens if Ruby fails to expand_path the value of the variable, or the fallback value of ~/.config.
The bahavior is identical to the method dealing with ~/.rspec.

This comment has been minimized.

@JonRowe

JonRowe Apr 23, 2018

Member

Verified that this error would only happen with a weird variable set, but can we improve the message? This warning will only occur when $XDG_CONFIG_HOME is malformed.

This comment has been minimized.

@jasonkarns

jasonkarns Apr 23, 2018

@myronmarston

What does the HOME environment variable have to do with XDG_CONFIG_HOME?

Not speaking to this particular verbiage, but HOME/.config is the default value for XDG_CONFIG_HOME.

From the spec:

If $XDG_CONFIG_HOME is either not set or empty, a default equal to $HOME/.config should be used.

This comment has been minimized.

@myronmarston

myronmarston Apr 24, 2018

Member

It's still unclear to me what method call is triggering the ArgumentError, which makes it hard to understand in what condition the warning is issued. Can you refactor so the rescue is directly around the method call that can raise ArgumentError?

This comment has been minimized.

@JonRowe

JonRowe Apr 24, 2018

Member

@myronmarston it's the call to File.expand_path, Ruby will raise an ArgumentError trying to expand ~ if the HOME environment variable is not set.

This comment has been minimized.

@Mange

Mange Apr 25, 2018

Contributor

According to the tests, this is also something that only happens on Ruby < 2.4.

# On Ruby 2.4, `File.expand("~")` works even if `ENV['HOME']` is not set.
# But on earlier versions, it fails.
it "warns when HOME env var is not set", :unless => (RUBY_PLATFORM == 'java' || RSpec::Support::OS.windows? || RUBY_VERSION >= '2.4') do
without_env_vars 'HOME' do
expect_warning_with_call_site(__FILE__, __LINE__ + 1)
RSpec::Core::ConfigurationOptions.new([]).options
end
end

@Mange

This comment has been minimized.

Contributor

Mange commented Apr 23, 2018

I see. Thank you for the feedback. Let's see if we can come up with something better together! ❤️

I based the message on the sister method that expands ~/.rspec, assuming that was the reason that expand_path could raise an ArgumentError. I really should have looked that up more, especially since I didn't consider the malformed value possibility.

I think we need to answer these questions:

  1. Should we support a value using the tilde-as-HOME syntax (e.g. ~/foo)?
  2. Can we tell exactly why expand_path fails if it fails and we decide to keep it? Should we abort or fall back to something or skip that file?
  3. Some examples in the tests talk about cases where HOME is not set. I cannot remember why right now and I'm actually pretty busy today and cannot check, but I vaguely remember it being a out some versions of Ruby or some OSes, something like that. How do they relate to this feature?

My suggestion is to lean towards just failing and showing details rather than trying to guess solutions and run with it. Probably remove the expand_path call.

`~/.rspec`, `.rspec`, `.rspec-local`, command line switches, and the
`SPEC_OPTS` environment variable (listed in lowest to highest precedence; for
example, an option in `~/.rspec` can be overridden by an option in
`.rspec-local`).

This comment has been minimized.

@jasonkarns

jasonkarns Apr 23, 2018

This wording seems mildly misleading. I'm assuming that rspec will only load one-of the xdg rspec file OR ~/.rspec, not both. Which means ~/.rspec won't be able to override any values from the xdg file location.

This comment has been minimized.

@JonRowe

JonRowe Apr 23, 2018

Member

Generally I believe they are cumulative (e.g. a global .rspec and local .rspec combine), indeed I would expect a local .rspec file to override this.

This comment has been minimized.

@jasonkarns

jasonkarns Apr 23, 2018

Right, the other existing file locations are cumulative. But I think XDG location should be considered the replacement for ~/.rspec. (And I presume ~/.rspec will eventually be deprecated in favor of the xdg-defined location.) As such, I feel like making these two cumulative sends the wrong message.

To clarify, I'd expect the xdg location to still be cumulative with local .rspec and all the rest. I'm specifically saying that just ~/.rspec and XDG_CONFIG_HOME/rspec/options should be mutually exclusive with each other.

This comment has been minimized.

@JonRowe

JonRowe Apr 23, 2018

Member

It will not be the replacement for ~/.rspec, not all systems utilise it. For example, OS X, Windows, therefore this can only be considered additionally supported.

This comment has been minimized.

@myronmarston

myronmarston Apr 23, 2018

Member

I think what @jasonkarns is saying makes sense. The 3 existing config files have specific purposes for all existing:

  • ~/.rspec is for personal options that apply to all RSpec projects
  • .rspec is for team options that apply to a specific project
  • ./rspec-local is for personal options that apply to a specific project

My understanding of XDG (and this PR) is that it's a more standardized system for providing what we're using ~/.rspec for. As such, for a given RSpec run, it doesn't really make sense (IMO) to merge ~/.rspec and the XDG rspec config file. If the user is setup to use the XDG config file, I think we should use it instead of the ~/.rspec file.

That said, I don't think the presence of the XDG_CONFIG_HOME env var is a sufficient signal to use the XDG file and ignore ~/.rspec, because someone could have XDG_CONFIG_HOME set for use by another tool, without realizing that RSpec will be using it, and could also have a ~/.rspec file that they expect to be honored. If we ignored ~/.rspec because XDG_CONFIG_HOME happened to be set on their machine it'd be bad.

So maybe the solution is to use the XDG config file if a file can be found, using either the env var or the default XDG path. If it can't be found, we fall back to ~/.rspec.

For example, OS X, Windows, therefore this can only be considered additionally supported.

My understanding (which may be wrong; I haven't read much about XDG) is that it's not an OS-specific thing. It's a standard that any tool can choose to honor, and any user can choose to use on any OS. If a user sets XDG_CONFIG_HOME and drops a config file in the configured path, and the tool is documented as supporting XDG config, it should load and use the user's config file, regardless of what OS they are on.

This comment has been minimized.

@JonRowe

JonRowe Apr 23, 2018

Member

👍 I just didn't want our assumed default here to be that we would deprecate the existing setup in favour of this :)

This comment has been minimized.

@jasonkarns

jasonkarns Apr 23, 2018

Works for me. Especially since rspec doesn't do any generation of the system-wide rspec file. (to my knowledge?)

My main concern for it to be deprecated was reminiscent of other tools that do generate their config files, and continued to write directly to ~/.foo despite XDG_CONFIG_HOME being set.

As long as rspec respects ${XDG_CONFIG_HOME:-$HOME/.config}/rspec/options when it exists, I'm 👍 😀

This comment has been minimized.

@Mange

Mange Apr 24, 2018

Contributor

Isn't it's easier to explain it as yet another file in the chain rather than a replacement for the HOME file if present?

What advantage do we get from ignoring the HOME file if both files exist?

I would have both files until all my projects were on a version of rspec that supports XDG, but they would be identical (HOME file would be a symlink to the XDG file) so I don't think I'd get anything from that.

If there are some tools out there that generates a HOME file, then it might be confusing for a user to get an effect not present in their XDG file, but it could also be confusing if they didn't get the effect despite the tool claiming to have installed the options. I cannot tell which is worse.

This comment has been minimized.

@jasonkarns

jasonkarns Apr 25, 2018

I'm not sure that the explanation is sufficiently more confusing to warrant making the files semantically ambiguous. The XDG location is semantically a replacement for the root home directory. Going against that would be a violation of the spirit of the XDG spec, IMO, if not a true violation of the spec. And I agree with @myronmarston 's comment that there are three distinct roles for the different files.

The 3 existing config files have specific purposes for all existing:

~/.rspec is for personal options that apply to all RSpec projects
.rspec is for team options that apply to a specific project
./rspec-local is for personal options that apply to a specific project

The XDG location is precisely the same role as ~/.rspec: "personal options that apply to all RSpec projects". Having two different files fill the same role would be more confusing, IMO.

This comment has been minimized.

@myronmarston

myronmarston Apr 25, 2018

Member

Having two different files fill the same role would be more confusing, IMO.

Agree. In addition, the precedence between the 3 existing files is easy to understand and make sense (e.g. personal options can override team ones, and local personal options can override global personal options). But with this new file fulfilling the same role as ~/.rspec, there's no basis for having ~/.rspec override the XDG file, or vice versa. It's arbitrary. I'd prefer not to make an arbitrary decision.

@jasonkarns

This comment has been minimized.

jasonkarns commented Apr 23, 2018

Should we support a value using the tilde-as-HOME syntax (e.g. ~/foo)?

I'm ambivalent on this. The XDG spec doesn't indicate this capability either way. Though anecdotally, it seems that a good majority of the XDG-compliant apps do support tilde expandsion of the XDG_CONFIG_HOME variable itself. Practically speaking, the way the code is currently written it makes this expansion automatically. (Since the ENV.fetch's fallback is using ~, the value will need expanded anyhow.) I rather like how explicit this makes the code: explicitly stating that ~/.config is the default value for XDG_CONFIG_HOME.

Can we tell exactly why expand_path fails if it fails and we decide to keep it? Should we abort or fall back to something or skip that file?

I can't imagine what kind of malformity would be possible that is somehow distinguishable from "path does not exist". So unless there is something abundantly clear in the exception, I'd probably expect any exception to be handled as if the expanded path were valid but didn't exist.

@JonRowe

This comment has been minimized.

Member

JonRowe commented Apr 23, 2018

I can't imagine what kind of malformity would be possible that is somehow distinguishable from "path does not exist".

This code doesn't check for an existing path.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Apr 24, 2018

@Mange do you want to get this PR green before we go through a full review (since that can potentially change the code a fair bit)?

Mange added some commits Apr 25, 2018

@Mange

This comment has been minimized.

Contributor

Mange commented Apr 25, 2018

Pushed some changes:

  1. Moved around the File.expand_path call so it is more isolated. Documented the failure condition, copied from the tests for the warning generated for ~/.rspec.
  2. No longer emitting a warning when failing to resolve $XDG_CONFIG_HOME, or ~/.config as ~/.rspec already emits that warning.
  3. Stopped using Pathname#write in favor of the older File.open(...) { } variant.

It looks like this will fix the build, but I don't have time to wait for the JRuby builds right now. I'll jump in and try to fix them too if they end up failing.

@Mange

This comment has been minimized.

Contributor

Mange commented May 1, 2018

Discussion seems to have died down. Let's summarize the current issues:

  • Build is red.
    • This is fixed!
  • The expand_path+rescue bit is confusing.
    • Should no longer apply; the rescue is more focused and produces no warning.
  • Should XDG file be a replacement for file in HOME?
    • I think the current consensus seems to be that if XDG file is present, then the HOME file should not be loaded. XDG file should be a replacement of HOME file.
    • Is there any objections to this? Anyone who wants to speak up before I make this change?
    • Question: If both files are present, should a warning be shown or should we save on boot time by not even looking for HOME file if we found a XDG file?
    • Question: What would be a good way of describing this in all the comments listing the files that will be loaded?
@jasonkarns

This comment has been minimized.

jasonkarns commented May 1, 2018

if XDG file is present, then the HOME file should not be loaded. XDG file should be a replacement of HOME file.
👍

If both files are present, should a warning be shown or should we save on boot time by not even looking for HOME file if we found a XDG file?

I imagine it would be quite common for people to have duplicate files for a while – specifically, while users have different projects using different versions of rspec. I could see most people making one a symlink of the other, but still. With Project A on latest rspec using XDG location, the homedir location would still be necessary for any other projects using an older version of rspec not yet supporting XDG. So I'd say XDG should take precedence and not even bother checking $HOME. Any scenarios that would be a problem with this approach?

lowest to highest precedence; for example, an option in `~/.rspec` can be
overridden by an option in `.rspec-local`).
* Global (XDG): `$XDG_CONFIG_HOME/rspec/options` (i.e. in the user's
[the XDG Base Directory

This comment has been minimized.

@myronmarston

myronmarston May 2, 2018

Member

"in the user's the XDG Base Directory Specification config directory" reads a bit funny. Maybe remove the 2nd "the"?

@myronmarston

This comment has been minimized.

Member

myronmarston commented May 2, 2018

So I'd say XDG should take precedence and not even bother checking $HOME. Any scenarios that would be a problem with this approach?

I think it's only problematic if the user has an XDG file and doesn't realize it. For example, if they install a common set of config files provided by their employer, and that includes an XDG config file for RSpec, it could be confusing. The user could have no idea it's there and get confused about why their settings in ~/.rspec are being ignored.

I don't think that's likely to be a common enough occurrence to warrant warning, though.

create_fixture_file("./.rspec", "--format project")
create_fixture_file("~/.rspec", "--format global")
create_fixture_file("~/.config/rspec/options", "--format xdg")
create_fixture_file("./custom.opts", "--force-color")

This comment has been minimized.

@myronmarston

myronmarston May 2, 2018

Member

Thanks for fixing this up!

squash! Add support for $XDG_CONFIG_HOME/rspec/options
XDG config file completely overrides HOME file.
@Mange

This comment has been minimized.

Contributor

Mange commented May 2, 2018

I've pushed some additions.

Now the code will ignore the ~/.rspec file is a XDG file is present and most of the documentation has been updated to explain this fact. I tried to make the documentation focused on the fact that there are several "layers" to the files, and that each file had a specific purpose. Let me know what you think about this.

@myronmarston

LGTM!

@myronmarston myronmarston merged commit 5e395e2 into rspec:master May 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@myronmarston

This comment has been minimized.

Member

myronmarston commented May 9, 2018

Thanks @Mange!

myronmarston added a commit that referenced this pull request May 9, 2018

@Mange Mange changed the title from Add support for XDG base directory support for configuration file to Add support for XDG base directory configuration file May 9, 2018

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