Try parse installables as store paths first #4569
Conversation
|
For what it's worth, we do flake builds exclusively, and have a moderately complex Node.js / PHP application that we build with Nix. I deployed |
| if (s.find('/') != std::string::npos) { | ||
| try { | ||
| result.push_back(std::make_shared<InstallableStorePath>(store, store->followLinksToStorePath(s))); | ||
| continue; | ||
| } catch (BadStorePath &) { | ||
| } catch (...) { |
There was a problem hiding this comment.
I think we shouldn’t drop every exception like that, but rather keep the old logic of storing the exception (unless it’s a BadStorePath) and eventually throwing it if both parsing methods failed
| @@ -722,3 +723,8 @@ git -C $flakeB commit -a -m 'Foo' | |||
|
|
|||
| # Test list-inputs with circular dependencies | |||
| nix flake metadata $flakeA | |||
|
|
|||
| # Test flake in store does not evaluate | |||
There was a problem hiding this comment.
The comment probably isn’t really clear out-of-context. Something like below might be clearer
| # Test flake in store does not evaluate | |
| # A store path containing a `flake.nix` should be treated as a store path, and not a flake |
Also it might be worth adding a test to ensure that for example file:/nix/store/foobar will be treated as a flake url and not a store path
|
Thanks, I've added the test from this PR, flipped the order of parsing as suggested by @regnat, and added a test that you can use |
Potential fix for #4568.
This simply flips the order of tests for flake / store path in
SourceExprCommand::parseInstallables. There was some extra logic to prioritise the parseFlake exception, but I don't believe we need it anymore.I was a little surprised this passed all tests. I'm not sure how much confidence that gives us, but figured I'd open a PR to discuss. Please consider I'm not very familiar with this code or the impact of this change.