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

Allow relative paths in --store option #3736

Merged

Conversation

@meditans
Copy link
Contributor

meditans commented Jun 23, 2020

In nix commands which accept --store options, we can now specify a
relative path, which will be canonicalized. The relative path must start
with ./ like in the nix language.

Fixes #3734

In nix commands which accept --store options, we can now specify a
relative path, which will be canonicalized.
@meditans meditans changed the title Allow relative paths in --store option WIP Allow relative paths in --store option Jun 23, 2020
@meditans
Copy link
Contributor Author

meditans commented Jun 23, 2020

It works, but it's marked as WIP as some tests should be added for this.

@@ -864,7 +864,7 @@ StoreType getStoreType(const std::string & uri, const std::string & stateDir)
{
if (uri == "daemon") {
return tDaemon;
} else if (uri == "local" || hasPrefix(uri, "/")) {
} else if (uri == "local" || hasPrefix(uri, "/") || hasPrefix(uri, "./")) {

This comment has been minimized.

Copy link
@regnat

regnat Jun 24, 2020

Contributor

Might make sense to also allow . as a path

This comment has been minimized.

Copy link
@meditans

meditans Jun 26, 2020

Author Contributor

The cost of that would be divergence from the nix expression language though, right?

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Jul 8, 2020

Member

this already happens with cli args. for instance:

nix build -f . hello

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jul 17, 2020

Member

but does -f <url> work? Because we are overloading URL and relative path syntax, and URL syntax already is infamous for having parser mistakes/ambiguities/vulnerabilities, I rather move slowly. We can always add . later.

Of course, if @edolstra wants / there's concensus to go ahead and do ., we'll happily add it, but I rather advocate for the smallest change that gets the job done.

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Jul 17, 2020

Member

url's do work (although they have to be in tar format):

nix build -f https://github.com/NixOS/nixpkgs/archive/master.tar.gz hello

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Jul 17, 2020

Member

rules for it are:

Path lookupFileArg(EvalState & state, string s)
{
if (isUri(s)) {
return state.store->toRealPath(
fetchers::downloadTarball(
state.store, resolveUri(s), "source", false).first.storePath);
} else if (s.size() > 2 && s.at(0) == '<' && s.at(s.size() - 1) == '>') {
Path p = s.substr(1, s.size() - 2);
return state.findFile(p);
} else
return absPath(s);
}

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jul 18, 2020

Member

OK fair, but it as wrong of me to compare -f <url> in the first place, because this sort of "each store parses the thing" means having else cases is no good either---everything should be if (...) { ... } else return nullptr.

Now we could still do ., yes. My above does not work argument against that, I will totally grant.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jul 20, 2020

Member

@edolstra's weak preference is if it doesn't contain :// and does contain / then it is deemed a path and is turned into a LocalStore.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jul 21, 2020

Member

In meeting we got @edolstra's preference, and I just updated the PR accordingly.

meditans added 3 commits Jun 26, 2020
@meditans meditans force-pushed the obsidiansystems:allow-relative-paths-in-store-option branch from 8e5be0c to 4178f36 Jul 17, 2020
@meditans meditans changed the title WIP Allow relative paths in --store option Allow relative paths in --store option Jul 17, 2020
The was Eelco's prefered logic, and it looks good to me!
@@ -914,12 +914,20 @@ ref<Store> openStore(const std::string & uri_,
throw Error("don't know how to open Nix store '%s'", uri);
}

static bool isNonUriPath(const std::string & spec) {

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Jul 21, 2020

Member

Can you just use !isUri from file-transfer?

@edolstra edolstra merged commit ff314f1 into NixOS:master Jul 21, 2020
2 checks passed
2 checks passed
tests (ubuntu-latest)
Details
tests (macos-latest)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.