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

replace the filepath argument in addToStore with a more common type NarSource (The core part) #177

Merged
merged 5 commits into from
Apr 28, 2022
Merged

replace the filepath argument in addToStore with a more common type NarSource (The core part) #177

merged 5 commits into from
Apr 28, 2022

Conversation

soulomoon
Copy link
Contributor

@soulomoon soulomoon commented Feb 8, 2022 •
edited
Loading

type NarSource m = (ByteString -> m ()) -> m ().

  • FilePath can turn to NarSource m.
  • Text can turn to NarSource m

So It could fix #176

@soulomoon soulomoon changed the title replace the filepath argument in addToStore with a more common type replace the filepath argument in addToStore with a more common type NarSource Feb 8, 2022
@soulomoon soulomoon mentioned this pull request Feb 8, 2022
Closed
@soulomoon
Copy link
Contributor Author

soulomoon commented Feb 8, 2022 •
edited
Loading

Choices on functions to convert string and filepath to nar is still need to be revised

@soulomoon soulomoon mentioned this pull request Feb 8, 2022
Merged
@soulomoon
Copy link
Contributor Author

path filter should take place in building narsource, remove from addToStore

@soulomoon soulomoon marked this pull request as ready for review February 9, 2022 11:11
@soulomoon
Copy link
Contributor Author

soulomoon commented Feb 9, 2022 •
edited
Loading

Some workflows failed.
It seems the dependency of hnix-store-remote depend on the released version of hnix-store-core, but the changes spans over both repositories.

I don't know what should be done under current situation in hnix-store.

@sorki
Copy link
Member

sorki commented Feb 9, 2022

Yes, that's intentional, see #101 - they are treated as separate packages and we need to first tackle core part PR, make a release and do the same for remote.

@soulomoon
Copy link
Contributor Author

cool, understand.

@Anton-Latukha
Copy link
Contributor

ifirc the core need to be updated & released before merging changes into remote. I waited, maybe you would do 2 PRs out of this one,

Or you consider this PR ready. The PR never was marked a draft, so seems its status never transmitted consideration of the readiness.

Is this PR considered ready?

@soulomoon
Copy link
Contributor Author

soulomoon commented Feb 15, 2022 •
edited
Loading

@Anton-Latukha , The updated remote and updated core is compatible as I tested in local. But the current PR would break the pipeline. I would stash the remote part changes and make the pipelines work. Then after this PR is merged, pop the stash to make another PR.

Stashing is done, please run the workflows. I think it would be ready to merge if the pipelines success

@soulomoon soulomoon changed the title replace the filepath argument in addToStore with a more common type NarSource replace the filepath argument in addToStore with a more common type NarSource (The core part) Feb 15, 2022
@Anton-Latukha
Copy link
Contributor

Ok. Today is late, would attend tomorrow, if the situation would allow.

I'd preferred @sorki to review hnix-store things, because I know part of hnix-store, but sorki has deeper knowledge of it & of Nix processes there.

And also the more I involve with hnix-store - the more I would take responsibility for hnix-store the more I would want to refactor it to the layout form that HNix is currently in. I'd loved to move Nix.Utils under all Nix projects & use some of the most helpful ones in the store code. I try to abstain from doing refactoring and with sorki we kind of talked about it back in the day.

@soulomoon soulomoon mentioned this pull request Feb 20, 2022
Closed
@sorki
Copy link
Member

sorki commented Mar 7, 2022

Can you please rebase due to CI and maybe squash the minor fixes into respective commits?

@soulomoon
Copy link
Contributor Author

soulomoon commented Mar 7, 2022 •
edited
Loading

Can you please rebase due to CI and maybe squash the minor fixes into respective commits?

Yeah, sure, I will adjust them later on.

@sorki rabase done, see if it passes the CI now

@soulomoon @sorki
replace the filepath argument in addToStore with a more common type…
0758bc3 
… `type NarSource m = (ByteString -> m ()) -> m ()`.

    * `FilePath` can turn to `NarSource m`.
    * `Text` can turn to `NarSource m`
@soulomoon @sorki
Rmove path filtering from addToStore
aa0287a 
Since NarSource, It should be done building NarSource

format
@soulomoon @sorki
exposing dumpString in Nix.Nar
c72c832 
remove trailling whitespace
@soulomoon @sorki
rename filePathToNar to dumpPath
c90b261 
and move it to Nix.Nar
@soulomoon @sorki
add doc and refactor
e3723c7
@sorki
Copy link
Member

sorki commented Apr 22, 2022

Somehow I've managed to miss the notification - there were some remnants of merges so I filtered this to only touch -core and they went away. CI broke meanwhile, so fixed that and rebased again and all looks good now.

@sorki sorki merged commit 71991e5 into haskell-nix:master Apr 28, 2022
@soulomoon soulomoon mentioned this pull request Aug 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anton-Latukha Anton-Latukha Anton-Latukha 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.

Current addToStore API from hnix-store is too limited, we should shape it
3 participants
@soulomoon @sorki @Anton-Latukha