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

Low level <drvPath>^<outputName> installable syntax to match existing <highLevelInstallable>^<outputNames> syntax #4543

Merged
merged 30 commits into from
Dec 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8499f32
New "indexed" installable syntax: `<drvPath>!<outputName>`
Ericson2314 Feb 12, 2021
1ef88da
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Sep 30, 2021
e5c42bb
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Mar 10, 2022
0966532
Merge remote-tracking branch 'upstream' into indexed-store-path-outputs
Ericson2314 Mar 25, 2022
9c6be01
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Apr 1, 2022
6951b26
Require (new) computed-derivations experimental feature for ! install…
Ericson2314 Apr 1, 2022
5c1f2e0
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Apr 7, 2022
fda2224
Add release notes mark experimental
Ericson2314 Apr 7, 2022
41e755b
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Apr 19, 2022
6b61d77
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Apr 19, 2022
b18720e
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 May 12, 2022
49ad315
Use `^` not `!` in indexed store derivations installable syntax
Ericson2314 May 12, 2022
b585548
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Jun 2, 2022
6cafe30
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Jul 14, 2022
f3262bc
Combine `InstallableStorePath` with `InstallableIndexedStorePath`
Ericson2314 Jul 14, 2022
8735f55
Fix bug, test more, document more
Ericson2314 Jul 15, 2022
279ecf7
Remove `computed-derivations` experimental feature
Ericson2314 Jul 15, 2022
0e4ec98
Fix typo in docs
Ericson2314 Jul 15, 2022
12461e2
Leverage existing docs for new store-path^outputs syntax
Ericson2314 Jul 15, 2022
13f2a6f
Merge branch 'master' into indexed-store-path-outputs
Ericson2314 Oct 28, 2022
26534f1
Merge branch 'master' into indexed-store-path-outputs
Ericson2314 Nov 25, 2022
1879c7c
Merge branch 'master' into indexed-store-path-outputs
Ericson2314 Dec 12, 2022
dc075dc
Apply suggestions from code review
Ericson2314 Dec 12, 2022
c7cce3e
Improve release notes
Ericson2314 Dec 12, 2022
d8c1c24
Adjust docs
Ericson2314 Dec 12, 2022
c886b18
Merge new tests into `build.sh`
Ericson2314 Dec 12, 2022
dabb03b
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Dec 12, 2022
32ae715
Fix typos in the docs
Ericson2314 Dec 12, 2022
5273cf4
Merge remote-tracking branch 'upstream/master' into indexed-store-pat…
Ericson2314 Dec 12, 2022
f61d575
Merge branch 'indexed-store-path-outputs' of github.com:obsidiansyste…
Ericson2314 Dec 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions doc/manual/src/release-notes/rl-next.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,14 @@
'<nixpkgs>' -A hello
# NIX_PATH=nixpkgs=flake:nixpkgs nix-build '<nixpkgs>' -A hello
```

* Allow explicitly selecting outputs in a store derivation installable, just like we can do with other sorts of installables.
For example,
```shell-session
$ nix-build /nix/store/gzaflydcr6sb3567hap9q6srzx8ggdgg-glibc-2.33-78.drv^dev`
```
now works just as
```shell-session
$ nix-build glibc^dev`
```
does already.
77 changes: 52 additions & 25 deletions src/libcmd/installables.cc
Original file line number Diff line number Diff line change
Expand Up @@ -399,44 +399,56 @@ static StorePath getDeriver(
struct InstallableStorePath : Installable
{
ref<Store> store;
StorePath storePath;
DerivedPath req;
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved

InstallableStorePath(ref<Store> store, StorePath && storePath)
: store(store), storePath(std::move(storePath)) { }
: store(store),
req(storePath.isDerivation()
? (DerivedPath) DerivedPath::Built {
.drvPath = std::move(storePath),
.outputs = {},
Copy link
Member Author

Choose a reason for hiding this comment

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

Note it previously read the derivation and returned all its output names explicitly here, but that didn't work e.g. when the drv file itself needs to be downloaded and so we cannot yet read it.

}
: (DerivedPath) DerivedPath::Opaque {
.path = std::move(storePath),
})
{ }

InstallableStorePath(ref<Store> store, DerivedPath && req)
: store(store), req(std::move(req))
{ }

std::string what() const override { return store->printStorePath(storePath); }
std::string what() const override
{
return req.to_string(*store);
}

DerivedPaths toDerivedPaths() override
{
if (storePath.isDerivation()) {
auto drv = store->readDerivation(storePath);
return {
DerivedPath::Built {
.drvPath = storePath,
.outputs = drv.outputNames(),
}
};
} else {
return {
DerivedPath::Opaque {
.path = storePath,
}
};
}
return { req };
}

StorePathSet toDrvPaths(ref<Store> store) override
{
if (storePath.isDerivation()) {
return {storePath};
} else {
return {getDeriver(store, *this, storePath)};
}
return std::visit(overloaded {
[&](const DerivedPath::Built & bfd) -> StorePathSet {
return { bfd.drvPath };
},
[&](const DerivedPath::Opaque & bo) -> StorePathSet {
return { getDeriver(store, *this, bo.path) };
},
}, req.raw());
}

std::optional<StorePath> getStorePath() override
{
return storePath;
return std::visit(overloaded {
[&](const DerivedPath::Built & bfd) {
return bfd.drvPath;
},
[&](const DerivedPath::Opaque & bo) {
return bo.path;
},
}, req.raw());
}
};

Expand Down Expand Up @@ -803,7 +815,22 @@ std::vector<std::shared_ptr<Installable>> SourceExprCommand::parseInstallables(
for (auto & s : ss) {
std::exception_ptr ex;

if (s.find('/') != std::string::npos) {
auto found = s.rfind('^');
if (found != std::string::npos) {
try {
result.push_back(std::make_shared<InstallableStorePath>(
store,
DerivedPath::Built::parse(*store, s.substr(0, found), s.substr(found + 1))));
continue;
} catch (BadStorePath &) {
} catch (...) {
if (!ex)
ex = std::current_exception();
}
}

found = s.find('/');
if (found != std::string::npos) {
try {
result.push_back(std::make_shared<InstallableStorePath>(store, store->followLinksToStorePath(s)));
continue;
Expand Down
15 changes: 8 additions & 7 deletions src/libstore/derived-path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,16 @@ DerivedPath::Opaque DerivedPath::Opaque::parse(const Store & store, std::string_
return {store.parseStorePath(s)};
}

DerivedPath::Built DerivedPath::Built::parse(const Store & store, std::string_view s)
DerivedPath::Built DerivedPath::Built::parse(const Store & store, std::string_view drvS, std::string_view outputsS)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DerivedPath::Built DerivedPath::Built::parse(const Store & store, std::string_view drvS, std::string_view outputsS)
DerivedPath::Built DerivedPath::Built::parse(const Store & store, std::string_view s)

and then use parseOutputsSpec() to parse the ^ and the outputs. And then SourceExprCommand::parseInstallables() doesn't need to split on ^.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #6815 which I made so this deduplication can happen. Note that part of the complication is ! is still used in the remote store protocol for DerivedPath, so some trickiness is needed so ^ and ! are used for different use-cases, but we can do better de-duplicating than we do currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

@edolstra can we return to this after this PR? #6815 to deduplicate things has prooved to be much bigger than this. I would really appreciate being able to make this change and then do the clean up afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

A simpler patch is not available because we still need to parse with with both ! and ^, because the former is still used internally e.g. in the protocols.

Copy link
Member Author

Choose a reason for hiding this comment

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

#6815 now works! But I'll still keep it separately because that is much bigger than this.

{
size_t n = s.find("!");
assert(n != s.npos);
auto drvPath = store.parseStorePath(s.substr(0, n));
auto outputsS = s.substr(n + 1);
auto drvPath = store.parseStorePath(drvS);
std::set<std::string> outputs;
if (outputsS != "*")
if (outputsS != "*") {
outputs = tokenizeString<std::set<std::string>>(outputsS, ",");
if (outputs.empty())
throw Error(
"Explicit list of wanted outputs '%s' must not be empty. Consider using '*' as a wildcard meaning all outputs if no output in particular is wanted.", outputsS);
}
return {drvPath, outputs};
}

Expand All @@ -95,7 +96,7 @@ DerivedPath DerivedPath::parse(const Store & store, std::string_view s)
size_t n = s.find("!");
return n == s.npos
? (DerivedPath) DerivedPath::Opaque::parse(store, s)
: (DerivedPath) DerivedPath::Built::parse(store, s);
: (DerivedPath) DerivedPath::Built::parse(store, s.substr(0, n), s.substr(n + 1));
}

RealisedPath::Set BuiltPath::toRealisedPaths(Store & store) const
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/derived-path.hh
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ struct DerivedPathBuilt {
std::set<std::string> outputs;

std::string to_string(const Store & store) const;
static DerivedPathBuilt parse(const Store & store, std::string_view);
static DerivedPathBuilt parse(const Store & store, std::string_view, std::string_view);
nlohmann::json toJSON(ref<Store> store) const;

bool operator < (const DerivedPathBuilt & b) const
Expand Down
16 changes: 16 additions & 0 deletions src/nix/nix.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,13 @@ operate are determined as follows:
```

and likewise, using a store path to a "drv" file to specify the derivation:

```console
# nix build '/nix/store/gzaflydcr6sb3567hap9q6srzx8ggdgg-glibc-2.33-78.drv^dev,static'
```

* You can also specify that *all* outputs should be used using the
syntax *installable*`^*`. For example, the following shows the size
of all outputs of the `glibc` package in the binary cache:
Expand All @@ -177,6 +184,12 @@ operate are determined as follows:
/nix/store/q6580lr01jpcsqs4r5arlh4ki2c1m9rv-glibc-2.33-123-dev 44200560
```

and likewise, using a store path to a "drv" file to specify the derivation:

```console
# nix path-info -S '/nix/store/gzaflydcr6sb3567hap9q6srzx8ggdgg-glibc-2.33-78.drv^*'
```
* If you didn't specify the desired outputs, but the derivation has an
attribute `meta.outputsToInstall`, Nix will use those outputs. For
example, since the package `nixpkgs#libxml2` has this attribute:
Expand All @@ -189,6 +202,9 @@ operate are determined as follows:
a command like `nix shell nixpkgs#libxml2` will provide only those
two outputs by default.

Note that a store derivation (given by `.drv` file store path) doesn't have
any attributes like `meta`, and thus this case doesn't apply to it.

* Otherwise, Nix will use all outputs of the derivation.

# Nix stores
Expand Down
54 changes: 49 additions & 5 deletions tests/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ set -o pipefail
nix build -f multiple-outputs.nix --json a b --no-link | jq --exit-status '
(.[0] |
(.drvPath | match(".*multiple-outputs-a.drv")) and
(.outputs | keys | length == 2) and
(.outputs.first | match(".*multiple-outputs-a-first")) and
(.outputs.second | match(".*multiple-outputs-a-second")))
(.outputs |
(keys | length == 2) and
(.first | match(".*multiple-outputs-a-first")) and
(.second | match(".*multiple-outputs-a-second"))))
and (.[1] |
(.drvPath | match(".*multiple-outputs-b.drv")) and
(.outputs | keys | length == 1) and
(.outputs.out | match(".*multiple-outputs-b")))
(.outputs |
(keys | length == 1) and
(.out | match(".*multiple-outputs-b"))))
'

# Test output selection using the '^' syntax.
Expand Down Expand Up @@ -56,6 +58,48 @@ nix build -f multiple-outputs.nix --json 'e^*' --no-link | jq --exit-status '
(.outputs | keys == ["a", "b", "c"]))
'

# Test building from raw store path to drv not expression.

drv=$(nix eval -f multiple-outputs.nix --raw a.drvPath)
if nix build "$drv^not-an-output" --no-link --json; then
fail "'not-an-output' should fail to build"
fi

if nix build "$drv^" --no-link --json; then
fail "'empty outputs list' should fail to build"
fi

if nix build "$drv^*nope" --no-link --json; then
fail "'* must be entire string' should fail to build"
fi

nix build "$drv^first" --no-link --json | jq --exit-status '
(.[0] |
(.drvPath | match(".*multiple-outputs-a.drv")) and
(.outputs |
(keys | length == 1) and
(.first | match(".*multiple-outputs-a-first")) and
(has("second") | not)))
'

nix build "$drv^first,second" --no-link --json | jq --exit-status '
(.[0] |
(.drvPath | match(".*multiple-outputs-a.drv")) and
(.outputs |
(keys | length == 2) and
(.first | match(".*multiple-outputs-a-first")) and
(.second | match(".*multiple-outputs-a-second"))))
'

nix build "$drv^*" --no-link --json | jq --exit-status '
(.[0] |
(.drvPath | match(".*multiple-outputs-a.drv")) and
(.outputs |
(keys | length == 2) and
(.first | match(".*multiple-outputs-a-first")) and
(.second | match(".*multiple-outputs-a-second"))))
'

# Make sure that `--impure` works (regression test for https://github.com/NixOS/nix/issues/6488)
nix build --impure -f multiple-outputs.nix --json e --no-link | jq --exit-status '
(.[0] |
Expand Down