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 20 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
1 change: 1 addition & 0 deletions doc/manual/src/release-notes/rl-next.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
arguments will be ignored and the resulting derivation will have
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
`__impure` set to `true`, making it an impure derivation.

* Allow explicitly selecting outputs with *store derivations* installable syntax too.
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
77 changes: 52 additions & 25 deletions src/libcmd/installables.cc
Original file line number Diff line number Diff line change
Expand Up @@ -395,44 +395,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 @@ -798,7 +810,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
17 changes: 9 additions & 8 deletions src/libstore/derived-path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,24 +93,25 @@ 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};
}

DerivedPath DerivedPath::parse(const Store & store, std::string_view s)
{
size_t n = s.find("!");
size_t n = s.rfind("!");
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
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
21 changes: 18 additions & 3 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,9 +184,17 @@ operate are determined as follows:
/nix/store/q6580lr01jpcsqs4r5arlh4ki2c1m9rv-glibc-2.33-123-dev 44200560
```

* 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:
and likewise, again using a store path to a "drv" file to specify the derivation:
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved

```console
# nix path-info -S --eval-store auto --store https://cache.nixos.org '/nix/store/gzaflydcr6sb3567hap9q6srzx8ggdgg-glibc-2.33-78.drv^*'
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
```

* If you didn't specify the desired outputs, but the derivation comes
from an expression which has an attribute `meta.outputsToInstall`, Nix
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
will use those outputs. For example, since the package
`nixpkgs#libxml2` has this attribute:

```console
# nix eval 'nixpkgs#libxml2.meta.outputsToInstall'
Expand Down
43 changes: 43 additions & 0 deletions tests/build-explicit-output.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
source common.sh
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved

set -o pipefail

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"))))
'
12 changes: 7 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
1 change: 1 addition & 0 deletions tests/local.mk
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ nix_tests = \
ssh-relay.sh \
plugins.sh \
build.sh \
build-explicit-output.sh \
ca/nix-run.sh \
selfref-gc.sh ca/selfref-gc.sh \
db-migration.sh \
Expand Down