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.

Sign up for GitHub

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

Jump to bottom

Improve digests generation and parsing #64

Closed
wants to merge 4 commits into from
Closed

Improve digests generation and parsing #64

wants to merge 4 commits into from

Conversation

layus
Copy link
Contributor

@layus layus commented May 10, 2020

This pr is based on top of #59 and fixes the only two isuues I encountered while writing prim_derivationStrict (place here the pr number in hnix when created).

Only the last two commits are relevant here.

0099f6d has a proper Text -> SomeNamedDigest function. Thechnically base64 hashes are not handled, but I did not enouter them in my tests (builing the hello package)

488fe78 fixes some quirks in hashes computations discovered by patiently comparing hnix and nix-instantiate outputs (I learned even more about dwarffs and gdb in the process :-))

@sorki sorki mentioned this pull request Jun 12, 2020
Merged
@layus layus marked this pull request as ready for review July 24, 2020 20:27
@layus
Copy link
Contributor Author

layus commented Jul 24, 2020

@sorki rebased and augmented to fix recent additions. See "Fix parsing of cas path adresses" in particular. All references to hashType were removed.

@layus
Copy link
Contributor Author

layus commented Jul 24, 2020

But it breaks some of hnix, as now SomeNamedDigest also has to be a Valid digest.

@layus
Copy link
Contributor Author

layus commented Jul 24, 2020

Breakage fixed ! (I think, I did not manage to test :-/)

@layus
Copy link
Contributor Author

layus commented Jul 24, 2020

Of course, haskell-nix/hnix#554 depends on this :-)

layus
layus commented Jul 24, 2020
View reviewed changes
hnix-store-remote/src/System/Nix/Store/Remote/Parsers.hs
<?> "Invalid Base32 part"
digest <- parseDigest
_ <- "fixed:"
narHashMode <- (pure Recursive <$> "r:") <|> (pure RegularFile <$> "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not too sure here. Maybe it's really "true" and "false" ? In mkFixedOutputDerivation we also have a name after fixed:, as in fixed:out:...

How can I properly test this ?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good, in my repo I have a similar fix pending:

narHashMode <- Data.Attoparsec.ByteString.Char8.option RegularFile (pure Recursive <$> "r:")

but I've stopped at the digest since it required changes from this PR.

For testing this I'll draft hnix-store-db PR shortly.

sorki
sorki reviewed Jul 27, 2020
View reviewed changes
hnix-store-remote/src/System/Nix/Store/Remote/Parsers.hs
Comment on lines +36 to +37
_ <- "text:sha256:"
digest <- decodeBase32 @'SHA256 <$> parseHash
Copy link
Member

Choose a reason for hiding this comment

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

Also had this one simplified:

Suggested change
_ <- "text:sha256:"
digest <- decodeBase32 @'SHA256 <$> parseHash
_ <- "text:"
digest <- parseTypedHash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept this one on purpose to fail on "text:xxx" where xxx is not sha256, which is the only allowed value.
We could however decide to be permissive here. Not sure which is best.

@sorki sorki mentioned this pull request Jul 27, 2020
Open
@layus layus mentioned this pull request Oct 27, 2020
Merged
This was referenced Nov 1, 2020
Draft
Merged
layus added 4 commits November 8, 2020 16:07
@layus
Add mkNamedDigest to parse text digests
85b75bc
@layus
Fix hash computation in makeFixedOutputPath
f42391f
@layus
Fix parsing of cas path adresses
2a6143a
@layus
Do not break everyone's code
dd70965 
Force named hashes to be valid. After all, what would be the point of
naming an invalid hash ?
This avoids propagating manually the ValidAlgo requirement everywhere.
@sorki
Copy link
Member

sorki commented Nov 9, 2020

Superseded by #73.

@sorki sorki closed this Nov 9, 2020
@layus layus deleted the storepath branch November 10, 2020 21:40
Anton-Latukha added a commit that referenced this pull request Jun 9, 2021
@Anton-Latukha
treewide: migrate crypto(hash-* -> nite)
c537587 
Reference:

Main cause:

haskell-hvr/cryptohash-sha512#7

The whole `cryptohash-*` package family is abandoned, there is no
signs of maintainer activity there, so it stopped following Haskell ecosystem &
`base` releases. Knowing the human history & situation around it - it would not
be reviwed, which gives experience to not hardcode on the (specifically when
emotional) dependency.

Experience I drawn from this story is to keep things simplier when possible &
have more flexible systems as a result code.
It was "a bit too much" for what hashing is, for the code to have 2 hashing type
systems (external & internal) & reinventment of `HashAlgorithm` type duplicate.
The whole code was really rigid with a lot of type applicating the data kinds,
those are dependent type features & should be used cautiously, since interface
became rigid to changes, so afterwards it is easier to dismantle and recreate
the subsystem then to evolve it.

Previous hashing history:
#156
#142
#93
#92
#90
#83
#64
#38
#32
#31
#28
#27
#25
#18
#14
Anton-Latukha added a commit that referenced this pull request Jun 9, 2021
@Anton-Latukha
treewide: migrate crypto(hash-* -> nite)
9804fd1 
Reference:

Main cause:

haskell-hvr/cryptohash-sha512#7

The whole `cryptohash-*` package family is abandoned, there is no
signs of maintainer activity there, so it stopped following Haskell ecosystem &
`base` releases. Knowing the human history & situation around it - it would not
be reviwed, which gives experience to not hardcode on the (specifically when
emotional) dependency.

Experience I drawn from this story is to keep things simplier when possible &
have more flexible systems as a result code.
It was "a bit too much" for what hashing is, for the code to have 2 hashing type
systems (external & internal) & reinventment of `HashAlgorithm` type duplicate.
The whole code was really rigid with a lot of type applicating the data kinds,
those are dependent type features & should be used cautiously, since interface
became rigid to changes, so afterwards it is easier & effective to dismantle
and recreate the subsystem then to evolve it.

Previous hashing history:
#156
#142
#93
#92
#90
#83
#64
#38
#32
#31
#28
#27
#25
#18
#14
Anton-Latukha added a commit that referenced this pull request Jun 9, 2021
@Anton-Latukha
treewide: migrate crypto(hash-* -> nite)
097af57 
Reference:

Main cause:

haskell-hvr/cryptohash-sha512#7

The whole `cryptohash-*` package family is abandoned, there is no
signs of maintainer activity there, so it stopped following Haskell ecosystem &
`base` releases. Knowing the human history & situation around it - it would not
be reviwed, which gives experience to not hardcode on the (specifically when
emotional) dependency.

Experience I drawn from this story is to keep things simplier when possible &
have more flexible systems as a result code.
It was "a bit too much" for what hashing is, for the code to have 2 hashing type
systems (external & internal) & reinventment of `HashAlgorithm` type duplicate.
The whole code was really rigid with a lot of type applicating the data kinds,
those are dependent type features & should be used cautiously, since interface
became rigid to changes, so afterwards it is easier & effective to dismantle
and recreate the subsystem then to evolve it.

Previous hashing history:
#156
#142
#93
#92
#90
#83
#64
#38
#32
#31
#28
#27
#25
#18
#14
Anton-Latukha added a commit that referenced this pull request Jun 10, 2021
@Anton-Latukha
treewide: migrate crypto(hash-* -> nite)
3655f75 
Reference:

Main cause:

haskell-hvr/cryptohash-sha512#7

The whole `cryptohash-*` package family is abandoned, there is no
signs of maintainer activity there, so it stopped following Haskell ecosystem &
`base` releases. Knowing the human history & situation around it - it would not
be reviwed, which gives experience to not hardcode on the (specifically when
emotional) dependency.

Experience I drawn from this story is to keep things simplier when possible &
have more flexible systems as a result code.
It was "a bit too much" for what hashing is, for the code to have 2 hashing type
systems (external & internal) & reinventment of `HashAlgorithm` type duplicate.
The whole code was really rigid with a lot of type applicating the data kinds,
those are dependent type features & should be used cautiously, since interface
became rigid to changes, so afterwards it is easier & effective to dismantle
and recreate the subsystem then to evolve it.

Previous hashing history:
#156
#142
#93
#92
#90
#83
#64
#38
#32
#31
#28
#27
#25
#18
#14
Anton-Latukha added a commit that referenced this pull request Jun 10, 2021
@Anton-Latukha
treewide: migrate crypto(hash-* -> nite)
97146b4 
Reference:

Main cause:

haskell-hvr/cryptohash-sha512#7

The whole `cryptohash-*` package family is abandoned, there is no
signs of maintainer activity there, so it stopped following Haskell ecosystem &
`base` releases. Knowing the human history & situation around it - it would not
be reviwed, which gives experience to not hardcode on the (specifically when
emotional) dependency.

Experience I drawn from this story is to keep things simplier when possible &
have more flexible systems as a result code.
It was "a bit too much" for what hashing is, for the code to have 2 hashing type
systems (external & internal) & reinventment of `HashAlgorithm` type duplicate.
The whole code was really rigid with a lot of type applicating the data kinds,
those are dependent type features & should be used cautiously, since interface
became rigid to changes, so afterwards it is easier & effective to dismantle
and recreate the subsystem then to evolve it.

Previous hashing history:
#156
#142
#93
#92
#90
#83
#64
#38
#32
#31
#28
#27
#25
#18
#14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorki sorki sorki left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@layus @sorki