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

Hash always has a valid type #3738

Merged
merged 16 commits into from Jul 27, 2020
Merged

Conversation

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 23, 2020

@edolstra based on your passing remark on our call, we made some of the ValidPathInfo/NarInfo hashes std::optional, but we're not really sure what the correct "bussiness logic" is, so that will be important to review.

These two remaining test failures may also turn up interesting things. edit nope, they weren't.

The parser changes use some of the same techniques I used in #3730 -- lots of std::string_view including a rest I mutate as I go----in general trying to write the code as if I had proper parser combinators at my disposal.

The rest is rote.

@Ericson2314 Ericson2314 marked this pull request as draft Jun 23, 2020
@Ericson2314 Ericson2314 changed the title WIP: Hash always has a valid type Hash always has a valid type Jun 23, 2020
@Ericson2314 Ericson2314 marked this pull request as ready for review Jun 23, 2020
@@ -115,7 +115,8 @@ struct ValidPathInfo
{
StorePath path;
std::optional<StorePath> deriver;
Hash narHash;
// TODO document this
std::optional<Hash> narHash;

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jun 24, 2020

Author Member

Here is one of the now-optional hashes

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jun 29, 2020

Author Member

@edolstra So I checked and while this one is optional, the constructor for NarInfo (derived class) requires that it is present with assertions. Is that good enough for now, or do you also want the refactor so that it is required everywhere? (I do recall you were saying the nix installer trick where the nar hash would be missing is long gone.)

(I must admit due to #3640 I am a bit partial to keeping it optional, but I'm happy to do whatever you like.)

This comment has been minimized.

Copy link
@edolstra

edolstra Jul 7, 2020

Member

I think this one should not be optional since a store path should always have a NAR hash.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jul 7, 2020

Author Member

This is actually turns out to be a fairly substantial change, because it looks like the BinaryCacheStore::addToStore overload that doesn't take the ValidPathInfo was uploading flat files as-is and calling them nars, with the narHash unset.

I think that a bug not intended feature, but either way, fixing it is the sort of major behavior change I rather do separately from this refactor which has no behavior changes?

@@ -11,7 +11,7 @@ namespace nix::fetchers {

struct TreeInfo
{
Hash narHash;
std::optional<Hash> narHash;

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jun 24, 2020

Author Member

Another

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jun 24, 2020

Author Member

Though I think this one is fine and just means the hash is calculted---obviously these builtins aren't going to be making non-CA paths.

This comment has been minimized.

Copy link
@edolstra

edolstra Jul 7, 2020

Member

I think this one should not be optional. Anyway it's better to leave libfetchers unchanged since it's all rewritten in the flakes branch.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jul 7, 2020

Author Member

We thought it might be easy to fix this, but it isn't either: the caching stuff doesn't store the narHash and since the cache also doesn't distinguish between stores, doing a queryPathInfo to get the Nar hash in the case of a cache-hit doesn't work reliably enough to pass the test suite.

Perhaps something could be made to work, but like you said libfetchers is all rewritten anyways, and I think the 2 lines we changed in libfetchers with the std::optional is much more minimal than whatever it would take to make it work without the std::optional.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jul 16, 2020

Author Member

This change did go away with the flakes branch being merged, yay!

@@ -10,7 +10,7 @@ struct NarInfo : ValidPathInfo
{
std::string url;
std::string compression;
Hash fileHash;
std::optional<Hash> fileHash;

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jun 24, 2020

Author Member

another

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Jun 29, 2020

Author Member

Per meeting this one is OK

Ericson2314 added 2 commits Jun 29, 2020
The aim of this check was just to ensure each key occurs once.
@Ericson2314 Ericson2314 force-pushed the obsidiansystems:hash-always-has-type branch from 058a156 to 1be279a Jul 2, 2020
@Ericson2314 Ericson2314 changed the title Hash always has a valid type WIP: Hash always has a valid type Jul 7, 2020
@meditans meditans force-pushed the obsidiansystems:hash-always-has-type branch from afe35d6 to dbffd30 Jul 7, 2020
@Ericson2314 Ericson2314 changed the title WIP: Hash always has a valid type Hash always has a valid type Jul 7, 2020
@edolstra
Copy link
Member

edolstra commented Jul 27, 2020

There are some conflicts, could you have a look at those? Otherwise LGTM.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 27, 2020

OK, fixed! Will get the PRs that include this too.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 27, 2020

Again?! :D

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 27, 2020

Fixed. Guess I shouldn't complain as it's my own other PR which caused the conflict :)

@edolstra edolstra merged commit 86805a2 into NixOS:master Jul 27, 2020
2 checks passed
2 checks passed
tests (ubuntu-latest)
Details
tests (macos-latest)
Details
@Ericson2314 Ericson2314 deleted the obsidiansystems:hash-always-has-type branch Jul 27, 2020
@B4dM4n
Copy link
Contributor

B4dM4n commented Jul 30, 2020

This PR introduced a evaluation error for me when building the niv package:

% nix run 'nix/86805a2c0a25f5ceefac0d64e64ba57ace73b7f5' -- build -f https://github.com/nmattia/niv/archive/c0a61e6283933fa6eed6d3451ffd86c9df5209ec.tar.gz -v --show-trace
error: --- Error ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- nix
store path mismatch in (possibly filtered) path added from '/nix/store/p46a908qp9pg5qdvm0v20kwisqixjdcq-source'
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- show-trace ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
trace: while evaluating 'sourceByRegex'
at: (8:30) in file: /nix/store/p46a908qp9pg5qdvm0v20kwisqixjdcq-source/default.nix

     7|
     8|   sourceByRegex = name: src: regexes:
      |                              ^
     9|     builtins.path {

trace: from call site
at: (22:16) in file: /nix/store/p46a908qp9pg5qdvm0v20kwisqixjdcq-source/default.nix

    21|
    22|   niv-source = sourceByRegex "niv" ./. [
      |                ^
    23|     "^package.yaml$"

The same build was successful with a5f7d31 (one commit before):

nix run 'nix/a5f7d310dd10fe86b6f6aa1c2771c30f113741d4' -- build -f https://github.com/nmattia/niv/archive/c0a61e6283933fa6eed6d3451ffd86c9df5209ec.tar.gz -v --show-trace

Not sure if niv is using builtins.path in a wrong way (default.nix#L9) and it needs to be fixed on their side.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 30, 2020

Thanks for reporting this, I do think it's a bug on the Nix side I'm sorry to have introduced. #3845 is the next step of this process, and I think will root out any renaming bugs, but if that can't be done fast enough we can separately investigate this one

@B4dM4n
Copy link
Contributor

B4dM4n commented Jul 31, 2020

The build failure is fixed in #3881. Thanks for your effort.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 31, 2020

Actually was totally coincidental! (As far as I know.) Thanks, Matt!

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.

None yet

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