Remote store #59

sorki · 2020-03-05T11:21:04Z

Summary

restores most functionality and tests
adds proper path parsing and quickcheck props
drops TypeLits and symbolic store path root
includes Base16/Base32 decoding, props/tests #57 (please comment on Base32 there) and Add makeFixedOutputPath #36 so this can be tested easily as a whole
addToStoreNar now works and has a test
drops regex-tdfa, uses simpler solution, original regex wouldn't roundtrip and it was the only use of regex-tdfa

The reason for dropping symbolic StoreDir is that knowing path at compile time seems too restrictive and prevents us working with any store root (as the test do - tests run namespaced and create and-hoc store in temporary directory not to interfere with systems nix store). Maybe there is a way to have this level of type safety on top of store implementations but making them part of StorePath complicates things quite a lot. If anyone see a better solution I'm open to ideas.

Can also split this into multiple commits if that would be better.

sorki · 2020-03-05T11:22:47Z

PTAL @imalsogreg @shlevy @stolyaroleh @puffnfresh @layus

layus

Whaaaow ! An impressive piece of work. Just nitpicks for now, but I will definitely try it later.

hnix-store-remote/src/System/Nix/Store/Remote.hs

stolyaroleh

👍 - I like the simpler StorePath type.

stolyaroleh · 2020-03-05T14:02:08Z

hnix-store-remote/src/System/Nix/Store/Remote.hs

+addToStore name pth recursive algoProxy pfilter repair = do
+
+  -- TODO: Is this lazy enough? We need `B.putLazyByteString bs` to stream `bs`
+  bs  :: BSL.ByteString <- liftIO $ B.runPut . putNar <$> localPackNar narEffectsIO pth


Does it make sense to use a streaming library here (so that adding files or consuming logs incrementally uses the same mechanism)?

Yes, even NARs needs streaming library (#51).

hnix-store-remote/src/System/Nix/Store/Remote.hs

hnix-store-remote/src/System/Nix/Store/Remote/Types.hs

hnix-store-core/src/System/Nix/ReadonlyStore.hs

layus

Again, this is great work. Thanks !

Putting typed store paths aside is a wise move. We can still reintroduce them later on if we feel so.

hnix-store-core/src/System/Nix/Internal/Base32.hs

layus · 2020-03-05T20:52:37Z

hnix-store-core/src/System/Nix/Internal/StorePath.hs

+makeStorePathName :: Text -> Either String StorePathName
+makeStorePathName n = case validStorePathName n of
+  True  -> Right $ StorePathName n
+  False -> Left $ reasonInvalid n
+
+reasonInvalid n | n == ""            = "Empty name"
+reasonInvalid n | (T.length n > 211) = "Path too long"
+reasonInvalid n | (T.head n == '.')  = "Leading dot"
+reasonInvalid n | otherwise          = "Invalid character"
+
+validStorePathName "" = False
+validStorePathName n  = (T.length n <= 211)
+                        && T.head n /= '.'
+                        && T.all validChar n


It would make sense to merge the three functions into one. Such as to avoid duplication of all the conditions. Then validStorePathName = isRight . makeStorePathName.

Sounds good, I think I was going to use validStorePathName with Arbitrary but decided against that.

hnix-store-remote/src/System/Nix/Store/Remote/Util.hs

layus · 2020-03-05T21:29:09Z

hnix-store-remote/src/System/Nix/Store/Remote/Logger.hs

+
+              next <- go decoder
+              return $ next
+


This "async read" part of the protocol looks really weird. Any idea why it was implemented like that in the daemon ?

hnix-store-remote/src/System/Nix/Store/Remote/Binary.hs

hnix-store-remote/hnix-store-remote.cabal

hnix-store-remote/tests/NixDaemon.hs

hnix-store-remote/src/System/Nix/Store/Remote/Protocol.hs

domenkozar · 2020-04-19T18:33:57Z

cc @shlevy would be amazing if you can take a look at this.

layus

Hi,

I have finally finished my prim_derivationStrict implementation based on your PR. It wokrs perfectly well for writing derivations, generating store paths and fixedOutput store paths, and reading the derivations.

I did not test the building part about getting the build result and triggering builds.

However I think this PR is quite ready to merge, and should not be delayed for too long. Types seem consistent. Further fixes to the implementation can be added later on. What do you think ?

sorki · 2020-05-10T12:56:40Z

@layus Nice! Can you point me to the prim_derivationStrict impl? I've recently PRed Gabriella439/Haskell-Nix-Derivation-Library#4 that should make integrating nix-derivation into this lib quite easy regarding reuse of StorePath. If you're on IRC we are now on #hnix channel as well, might bridge the gitter hnix channel with that at some point. Would make coordination easy if you can join.

I also have few changes pending from yesterday to this PR but nothing that should clash with your commits it seems.

sorki · 2020-05-10T13:03:39Z

Fixed usage of obsolete ValidPath which was replaced by StorePathMetadata. Also changed a type of addToStore to accept hash typed pun, removed redundant Proxy and changed pathFilter type to (FilePath -> Bool) although it is unused for now.

layus · 2020-05-10T19:14:58Z

There is a big pain point with path filters in the sense that they are generally nix functions to be evaluated inside MonadNix, which itself relies on the store. Is there a way to cope with this circular dependency ?

layus · 2020-05-23T22:15:56Z

hnix-store-remote/src/System/Nix/Store/Remote.hs

+buildDerivation p drv buildMode = do
+  runOpArgs BuildDerivation $ do
+    putPath p
+    putDerivation drv


This design decision bugs me in the nix protocol. At that point, the .drv is already present in the store. Why not provide a buildDerivation(/nix/store/xxx.drv) ? I guess the reason is about garbage collection and such, but still. What is the point of writing .drv's to the store then ?

Or rather, why do we have to upload both the path and the drv itself ?

Never mind, I found the wopBuildPaths that does exactly that. wopBuildDerivation is pretty specific for "anonymous" builds and was introduced for hydra in NixOS/nix@1511aa9

Thanks for the commit link. I was a bit confused about that as well.

layus · 2020-05-23T22:39:07Z

hnix-store-remote/src/System/Nix/Store/Remote.hs

+    -- XXX: reason for this is unknown
+    -- but without it protocol just hangs waiting for
+    -- more data. Needs investigation
+    putInt 0


Only thing I could see is that libstore has a flush() operation. But possibly it's just putEnum that is not up to the task.

You might be right about putEnum since it's the only use of it.

Doesn't make much sense either, putEnum is just putInt . fromEnum so it's more like missing field. I don't care that much at this point as I'm playing with local store and daemon impl. so I'll be able to end-to-end test this without nix-daemon and the reason will probably become clear.

sorki · 2020-05-29T13:04:45Z

rebased
added few docstrings
cleaned-up imports
added parsing and building of ContentAddressableAddresses

StorePathMetadata signatures need bit more legwork (extracting public keys from nix.conf). Marking this as ready for review as I'm quite happy with it now.

sorki · 2020-05-29T13:08:25Z

There is a big pain point with path filters in the sense that they are generally nix functions to be evaluated inside MonadNix, which itself relies on the store. Is there a way to cope with this circular dependency ?

@layus I think that proper use of effects and possibly introducing a library like polysemy might allow us to resolve this and metadata caching as well. I'll open an issue for discussing this separately with the PoC I wrote recently.

sorki · 2020-06-12T07:19:35Z

Further improvements to currently too sha256 specific parsers require 0099f6d from #64 which depends on this.

sorki · 2020-06-12T07:41:19Z

Added Closes clause referencing relevant issues.

sorki · 2020-06-28T08:43:55Z

I'll do one final pass over this, add changelog for -remote, changelog entries for -core, split a testsuite a bit and merge this during the next week if there are no objections.

Closes haskell-nix#15, haskell-nix#16, haskell-nix#21, haskell-nix#22, haskell-nix#62.

Closes haskell-nix#20.

Disabled by default since it requires `nix-daemon` binary and Linux namespaces support. For development this can be enabled by ``` cabal configure --flag=io-testsuite ``` or by adding ``` flags: +io-testsuite ``` to `cabal.project.local` Enabled by `callCabal2nixWithOptions` in `overlay.nix` so it is tested by `nix-build` and `nix-shell` brings all test dependencies. This is fine on NixOS where `build-tool-depends: nix:nix-daemon` works and we have namespaces supported.

sorki · 2020-07-14T11:33:11Z

ChangeLogs done, added dff7527 which puts hnix-store-remote testsuite behind a flag so we don't run into problems on non-NixOS systems.

Final pass hopefully, leaving open for few more days if anyone wants to chime in.

sjakobi · 2020-07-14T11:42:49Z

@sorki Where can I find out more about the context and goal of this work?

Apparently this ~~fixes~~ addresses #22?

Is there any more information on what hnix-store-remote is than the bit in https://github.com/haskell-nix/hnix-store/tree/master/hnix-store-remote#hnix-store-remote?

Apologies if this is not a good place for these questions! I'm just a bit curious.

sorki · 2020-07-14T12:02:55Z

@sorki Where can I find out more about the context and goal of this work?

This is mostly an adaptation of previous work to changes in core that were added to support the remote part, except for dropping symbolic StoreDir which proved difficult to work with and in the end not that important due to gradual move to store root independent Nix stores.

Few missing bits were added as well, especially

parsing for StorePaths
integration of nix-derivation that uses our StorePath type

Currently the goal is to allow interacting with nix-daemon via so called remote protocol and provide interface for hnix, so the calls to nix-instantiate and nix-store --add can be migrated to this (like haskell-nix/hnix#554 which this PR should make a bit easier).

Apparently this ~~fixes~~ addresses #22?

I hope but the description is too vague :)

Is there any more information on what hnix-store-remote is than the bit in https://github.com/haskell-nix/hnix-store/tree/master/hnix-store-remote#hnix-store-remote?

Part of https://github.com/haskell-nix/hnix-store#rationale is good overview, now when you asked you reminded me it needs an update to reflect the current state since remote is fully functional now (Rationale calls it daemon but it's a client to existing nix-daemon).

sorki mentioned this pull request Mar 5, 2020

Bit too late #35

Closed

layus reviewed Mar 5, 2020

View reviewed changes

hnix-store-remote/src/System/Nix/Store/Remote.hs Outdated Show resolved Hide resolved

stolyaroleh reviewed Mar 5, 2020

View reviewed changes

layus reviewed Mar 5, 2020

View reviewed changes

hnix-store-core/src/System/Nix/ReadonlyStore.hs Outdated Show resolved Hide resolved

layus approved these changes Mar 5, 2020

View reviewed changes

stolyaroleh reviewed Mar 6, 2020

View reviewed changes

hnix-store-remote/src/System/Nix/Store/Remote/Binary.hs Show resolved Hide resolved

stolyaroleh reviewed Mar 6, 2020

View reviewed changes

hnix-store-remote/src/System/Nix/Store/Remote/Binary.hs Show resolved Hide resolved

imalsogreg reviewed Mar 6, 2020

View reviewed changes

hnix-store-remote/hnix-store-remote.cabal Outdated Show resolved Hide resolved

puffnfresh approved these changes Mar 9, 2020

View reviewed changes

hnix-store-remote/tests/NixDaemon.hs Outdated Show resolved Hide resolved

hnix-store-remote/src/System/Nix/Store/Remote/Protocol.hs Outdated Show resolved Hide resolved

sorki mentioned this pull request Mar 13, 2020

A new release for hnix-store-core? #56

Closed

sorki mentioned this pull request Apr 6, 2020

hnix-store-remote: Remove ExceptT, use DList to accumulate log. #63

Closed

sorki force-pushed the storepath branch from 6dc7897 to b6792ed Compare April 18, 2020 19:21

sorki force-pushed the storepath branch from b6792ed to d99692c Compare April 20, 2020 08:03

layus mentioned this pull request May 10, 2020

Improve digests generation and parsing #64

Closed

layus approved these changes May 10, 2020

View reviewed changes

sorki mentioned this pull request May 19, 2020

derivationStrict haskell-nix/hnix#554

Merged

Anton-Latukha mentioned this pull request May 21, 2020

Use hnix-store 0.2 haskell-nix/hnix#561

Closed

layus reviewed May 23, 2020

View reviewed changes

sorki force-pushed the storepath branch from bf5c019 to a993136 Compare May 29, 2020 12:59

sorki marked this pull request as ready for review May 29, 2020 13:04

sorki force-pushed the storepath branch from a993136 to 87109ab Compare June 7, 2020 07:12

sorki mentioned this pull request Jun 12, 2020

Remove store type index to StorePath #62

Closed

This was referenced Jun 12, 2020

hnix-store-remote code cleanup #22

Closed

Validate we can add a drv file to the store with existing daemon implementation #20

Closed

sorki force-pushed the storepath branch from f85f8b5 to 2b01156 Compare June 12, 2020 07:40

Anton-Latukha mentioned this pull request Jun 19, 2020

Please, include maintainers of this repositories into the haskell-nix group haskell-nix/hnix#636

Closed

sorki added 8 commits July 14, 2020 13:27

simple store path root, remote store rework

1f86211

Closes haskell-nix#15, haskell-nix#16, haskell-nix#21, haskell-nix#22, haskell-nix#62.

switch hackage category from System to Nix

1127725

core: add System.Nix.Derivation and tests

1076926

remote: add buildDerivation and test

5f33d97

Closes haskell-nix#20.

core: derive Ord for StorePathTrust

4ddea5b

core: Update ChangeLog

84cbca2

remote: Add ChangeLog.md

850bafd

sorki force-pushed the storepath branch from 2b01156 to dff7527 Compare July 14, 2020 11:28

sorki merged commit 59e08d4 into haskell-nix:master Jul 24, 2020

sorki mentioned this pull request Jul 27, 2020

addToStore to S3 and local binary cache #37

Open

Anton-Latukha mentioned this pull request Dec 22, 2020

Skip or fix tests in nix-like sanboxes #80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote store #59

Remote store #59

sorki commented Mar 5, 2020

sorki commented Mar 5, 2020

layus left a comment

stolyaroleh left a comment

stolyaroleh Mar 5, 2020

sorki Mar 6, 2020 •

edited

Loading

layus left a comment

layus Mar 5, 2020

sorki Mar 6, 2020

layus Mar 5, 2020

domenkozar commented Apr 19, 2020

layus left a comment

sorki commented May 10, 2020

sorki commented May 10, 2020

layus commented May 10, 2020

layus May 23, 2020

layus May 23, 2020

layus May 23, 2020

sorki May 29, 2020

layus May 23, 2020

sorki May 29, 2020

sorki May 29, 2020

sorki commented May 29, 2020

sorki commented May 29, 2020

sorki commented Jun 12, 2020

sorki commented Jun 12, 2020

sorki commented Jun 28, 2020

sorki commented Jul 14, 2020

sjakobi commented Jul 14, 2020 •

edited

Loading

sorki commented Jul 14, 2020

Remote store #59

Remote store #59

Conversation

sorki commented Mar 5, 2020

sorki commented Mar 5, 2020

layus left a comment

Choose a reason for hiding this comment

stolyaroleh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorki Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

layus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenkozar commented Apr 19, 2020

layus left a comment

Choose a reason for hiding this comment

sorki commented May 10, 2020

sorki commented May 10, 2020

layus commented May 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorki commented May 29, 2020

sorki commented May 29, 2020

sorki commented Jun 12, 2020

sorki commented Jun 12, 2020

sorki commented Jun 28, 2020

sorki commented Jul 14, 2020

sjakobi commented Jul 14, 2020 • edited Loading

sorki commented Jul 14, 2020

sorki Mar 6, 2020 •

edited

Loading

sjakobi commented Jul 14, 2020 •

edited

Loading