Bit too late #35
Bit too late #35
Conversation
|
digestText16 can be implemented by means of I've also added |
| @@ -34,12 +37,23 @@ processOutput = go decoder | |||
| case ctrl of | |||
| e@(Error _ _) -> return [e] | |||
| Last -> return [Last] | |||
| Read n -> do | |||
There was a problem hiding this comment.
StoreMonad now embeds Maybe Handle in its StateT so we can provide handle and use it to provide data when other side requests tunneling.
| @@ -76,6 +76,7 @@ class HasDigest (a :: HashAlgorithm) where | |||
| initialize :: AlgoCtx a | |||
| update :: AlgoCtx a -> BS.ByteString -> AlgoCtx a | |||
| finalize :: AlgoCtx a -> Digest a | |||
| hashName :: T.Text | |||
There was a problem hiding this comment.
This is a sensible place to put hashName (I think others have put this under its own typeclass, but simply using HashDigest seems better). HasDigest is not a great typeclass name under this change though. What's a name for this class that's neither (a) too specific for the methods it provides, (b) as general as "Hash"?
There was a problem hiding this comment.
I disagree that this should be under HasDigest (which I've renamed ValidAlgo in my branch). Not every hash has a canonical name, e.g. the truncated ones don't (and shouldn't!).
| -- | Convert any Digest to a base32-encoded string. | ||
| -- This is not used in producing store path hashes | ||
| printAsBase32 :: Digest a -> T.Text | ||
| printAsBase32 (Digest bs) = printHashBytes32 bs | ||
|
|
||
| -- | Print lowercased name of the hashing algorithm |
There was a problem hiding this comment.
Can we remove this function? It perfectly overlaps with hashName.
| adjustName n | outputId == "out" = n | ||
| adjustName (PathName name) | otherwise = PathName $ T.concat [ name, T.pack "-", outputId ] | ||
|
|
||
| type Recursive = Bool |
There was a problem hiding this comment.
Nit: data Recursive = Recursive | NotRecursive preferred over type alias.
There was a problem hiding this comment.
Actually:
-- | Whether the hash is computedover the serialisation of the output path
-- or over the contents of the single non-executable regular file
-- See thesis page 107
data OutputHashMode = Recursive | Flat
There was a problem hiding this comment.
This is still being broken up into manageable chunks,but see 974a944#diff-d6eea410e6c564641deccfa88ae99d1fR101
| makeStored :: StoreDir -> Path -> Stored Path | ||
| makeStored sl p = (sl, p) | ||
|
|
||
| type PathType = Text |
There was a problem hiding this comment.
Could we create a new datatype for this? I think it would be:
-- | Store object type is used when creating store paths, to differentiate
-- between paths for sources and derivation outputs (see thesis page 94)
data StoreObjectType =
Source
| Output Text -- ^ A build output (e.g. `Output "out"`)
or
-- | A single derivation build output (e.g. "out", "docs" etc)
newtype BuildOutputName = BuildOutputName { getBuildOutputName :: Text }
-- | Store object type is used when creating store paths, to differentiate
-- between paths for sources and derivation outputs (see thesis page 94)
data StoreObjectType =
Source
| Output BuildOutputName -- ^ A build output (e.g. `Output (BuildOutputName "out")`)
| -- build <s> string which is hashed and used in makeStorePath | ||
| -- <s> = "<pathType>:<hash_algo>:<base16_hash>:<storeDir>:<pathName>" | ||
| -- (exposed for testing purposes only) | ||
| makePathDigestString :: (HasDigest a) => PathType -> PathName -> Digest a -> StoreDir -> Text |
There was a problem hiding this comment.
If this function is only used in makeStorePath, can we move it into a where clause in makeStorePath? That could help to reduce the largish number of functions with similar-sounding names at the top level.
|
Lot of good stuff here, @sorki! |
| where | ||
| base = T.pack . takeBaseName . BSC.unpack . BSL.toStrict $ p | ||
| parts = T.breakOn "-" base | ||
| digest = Digest . BSC.pack . T.unpack . fst $ parts |
There was a problem hiding this comment.
I think this is incorrect and needs #34
|
I would like to rebase and fix this at some point - probably after the Path changes are merged. |
|
Superseded by #59. All of the stuff from review is now addressed. |
Lots of things mainly
Sorry for getting this out late but I've got stuck last time mainly due to derivations and misunderstanding of paths - caused some work duplication but it should be merge-able.