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

Implement 'addToStore' #53

Closed
wants to merge 3 commits into from
Closed

Implement 'addToStore' #53

wants to merge 3 commits into from

Conversation

layus
Copy link
Contributor

@layus layus commented Nov 29, 2019

This convinced me that the project is mature enough to get started with actual implementations of the store API.

Some limitations that I discovered:

  1. I needed an HashAlgorithm outside of the type system. I worked around the missing value by abusing algoName as in algoName @hashAlgo == "sha256".
  2. We cannot turn a String into a store path, because the store path has the prefix in its type. This means that we must know the prefix before reaching to the daemon, or find something to make the return value of addToStore more generic. This leads to more questions
    i. Does a daemon only handle one prefix ?
    ii. Is there a way to know the prefix used by a daemon before reaching to it ?

\cc @shlevy to get your first impressions, and possibly merge after some edits.

Copy link
Member

@shlevy shlevy left a comment

Choose a reason for hiding this comment

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

Cool, thank you!

nar <- liftIO $ localPackNar narEffectsIO srcPath -- TODO actually use filter.
runOpArgs AddToStore $ do
putByteStringLen $ strToN $ unStorePathName name
putBool $ not (recursive && algoName @hashAlgo == "sha256") -- backwards compatibility hack
Copy link
Member

Choose a reason for hiding this comment

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

Let's add an algo :: HashAlgorithm field to ValidAlgo instead?

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 tried that, but could not make it work, because then you need to define algo for Truncated, and you need to instantiate Eq for HashAlgorithm. I failed at both of these.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, I think the fundamental problem is we're trying to pass the hash algo at the type level at all. Why not just take a value-level hash argument here? It's not like we construct a Digest or anything ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need a NamedHash to get the pass the corresponding algoName to the daemon.
How do you type a function that takes a HashAlgorith whose corresponding type is a a NamedHash instance ? And how do you access this instance from the value.

We could just take a string, but that would be kinda defeating the purpose of all of this.

Copy link
Member

Choose a reason for hiding this comment

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

Argh, right. I'd say we should just have a value-level hashName function but then we'd want to exclude truncated hashes.

I'm torn between getting rid of this type level machinery altogether or separating out the base hash algorithms (sha, md5, etc.) from the digest types (base algo | truncated). Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I only ever programmed in haskell for the past "advent of code". I am therefore no expert in the habits of haskell programming.

But, if we want to mimic the remote store api as close as possible, then passing a real hash value as an argument would make sense. The current type annotation is strange.
Also, the Truncated stuff is weird to handle.
I was also surprised at how easy it is to create a "fake" hash. Turning the returned string into a correctly typed store path was very easy, but could be incorrect. A type system that can be easily escaped looks a bit like a "nullptr" in other languages.

May I propose to at least remove Truncated from the datatype ? It has no use outside of store paths while the other algos are used everywhere in nixpkgs.
That way we can make the resulting HashAlgorithms Eq.

I guess we will get a better idea of what to do after implementing several api calls.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I will give this separation a shot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The further I think about this, the more it seems that using algoName could make sense. What does not make sense however, is how to create a valid StorePath from the string returned by the nix daemon.

The way I do it is not valid, as it creates a Digest from the base32-encoded hash. If you print a path generated like that, it re-encodes the digest to base32, making it too long.

https://github.com/haskell-nix/hnix-store/pull/53/files#diff-002378d5e26f547b360b3e048e2d2f9fR81-R88

If you split Truncated from the hash methods, a good use case is to implement makeStorePath :: BS.ByteString -> Maybe (StorePath storeDir)

hnix-store-remote/src/System/Nix/Store/Remote.hs Outdated Show resolved Hide resolved
@layus
Copy link
Contributor Author

layus commented Nov 29, 2019

@shlevy Updated. The code is a mess, but i implemented most of the missing features, except testing the algo in the source code. It still relies on algoName for that.

@@ -252,7 +259,7 @@ localPackNar effs basePath = Nar <$> localPackFSO basePath
<*> narFileSize effs path'
<*> narReadFile effs path'
(True , _) -> fmap (Directory . Map.fromList) $ do
fs <- narListDir effs path'
fs <- filter (pathFilter . (path' </>)) <$> narListDir effs path'
Copy link
Collaborator

Choose a reason for hiding this comment

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

filter uses the predicate (FilePathFilter) to include elements that match. The Haddock comment on 249 suggests that localPathNar' excludes matches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I was not sure myself while wirting it, hence this confusion. But we have to scrape this whole implementation as the only filter that we want to use is a nix function, and hence wrapped in a MonadNix monad. This implementation cannot be used at all from what I have found.


where
type FilePathFilter = FilePath -> Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a matter of style, so feel free to ignore it! But, why add a type alias? I like to use them sparingly, since they are another thing for API users to have to learn, and they can pile up over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, style has definitely been overlooked while writing this. I wanted to achieve a minimal prototype while ignoring everything else. I would dare say this PR is not for merging, more for sharing ideas and POC implementation. Hopefully it will trigger some more progress HNix.

@layus
Copy link
Contributor Author

layus commented Jan 8, 2020

@shlevy Why not merge hnix-store in the hnix repo. Further development requires changes in both hnix and hnix store, often closely related.

My work is stalled now for everything turns out to be incredibly tangled, especially while trying to make the monads in hnix-store and hnix to work together.

My wip is in my dev branch layus/hnix@e4713cc, especially the snippet https://github.com/layus/hnix/blob/dev/src/Nix/Effects.hs#L244-L297 where we see the complexity of integrating both. Ideally, we would not have to call Store.runStore inside the hnix monad store, and we should be able to keep the daemon socket alive while continuing the nix evaluation.

The remaining of the wip in this branch, but it is now completely broken permalink

If I could get but only one daemon call working correctly, the rest would be much easier, being just an extension that mimics the first call. Would you try to give a shot at a complete implementation of addTextToStore for example ?

@Ericson2314
Copy link
Member

I agree with @layus that we should put things in the same repo, especially while stuff is in flux. (Of course, no packages should be combined; people should load multiple packages in one GHCi. stack can do that, and as it so happens we are finishing up obsidiansystems/obelisk#489 to be able to do it too.)

@sorki
Copy link
Member

sorki commented Jan 28, 2020

Nice!

While I was trying to rebase my PR #35 I've got stopped by tests (once again), while I figured out something like runStore :: FilePath -> FilePath -> MonadStore ("lal" :: StoreDir) a -> IO (Either String a, [Logger]) I don't think that type level StoreDir is a correct solution as it enforces to knowing store paths ahead. Tests for example use ad-hoc temp directory not to interfere with system store.

Maybe I'm just missing something and there's a way to do it with TypeLits as well.

The other problems I found is with store path en/decoding IIRC but I need to look more closely again.

@layus
Copy link
Contributor Author

layus commented Nov 1, 2020

I think all the features here have been added in other PRs in the meantime. Closing

@layus layus closed this Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants