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

Start implementing #3006 #3120

Merged

Conversation

alexeyzab
Copy link
Collaborator

@alexeyzab alexeyzab commented Apr 12, 2017

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

This changes GitSHA1 from using a ByteString to a StaticSize 40 ByteString. Other edits make sure the code compiles and the tests pass.

It was not possible to derive a Hashable instance after the change, so I've implemented one by hand.

I have commented out the implementation of peek and poke for GitSHA1's Store instance as I am not sure if those are needed or not.

If these changes are satisfactory, I'll start working on the Stack.Types.PackageIndex.PackageDownload.pdSHA256's instances as well.

@alexeyzab
Copy link
Collaborator Author

I went ahead and made the changes to Stack.Types.PackageIndex.PackageDownload.pdSHA256. If this needs more work, let me know.

@mgsloan
Copy link
Contributor

mgsloan commented Apr 23, 2017

Hey! Sorry for the delay on feedback. Looks good to me, thanks for working on this! One thing is that the version tags, like (storeVersionConfig "pkg-v2" ...) should have its name updated (so, pkg-v2), since this will actually cause a difference in serialization. With the StaticSize version, the size won't get stored in the file at all.

…tras

Don't clean packages listed as extra-deps
@alexeyzab
Copy link
Collaborator Author

@mgsloan To clarify, should it be changed pkg-v3? I've adjusted the hash, but didn't touch this part.

On a separate note, should I remove the commented out definitions of poke and peek for GitSHA1? I put them in just in case they might be necessary here.

Thanks!

snoyberg and others added 17 commits May 1, 2017 12:31
…ad-cleanup

Upload cleanup, and save-hackage-creds option
…haskell#3159)

* Fixed pid1 ownership inside fpco/stack-build docker image
Because `--local-bin-path` is deprecated in favour of `--local-bin`.
…l#3158)

This seems a bit more accurate: When the user is already running the most recent version, the message "your version is already *more* recent" is incorrect and ever so slightly confusing. The other case -- where the user is running a more recent version than is available as a binary, for which the old message is accurate -- likely occurs much less frequently.
path-0.5.13, released in March 2017, [adds function
reldir](https://hackage.haskell.org/package/path-0.5.13/changelog).

Local variable reldir shadows it:
https://travis-ci.org/commercialhaskell/stack/jobs/234552558

So the combination doesn't build since March. Sorry this is the quickest fix
possible, I know a few alternatives, but I'm short on time and the choice is a
matter of taste.
@snoyberg
Copy link
Contributor

I totally broke this PR with extensible-snapshots, which switched from GitSHA1 to a SHA256. I'm going to this it up and open a new PR, I'll add a comment when that's done.

@snoyberg
Copy link
Contributor

PR #3260 is now open. When it's merged, it will merge this PR too.

@snoyberg
Copy link
Contributor

Continuing the discussion from the other PR: here's a really hacky implementation of the Word64 idea I mentioned: https://github.com/snoyberg/static-bytes

@alexeyzab
Copy link
Collaborator Author

That's pretty interesting. If we were to use that library, we'd change StaticSHA256 to something like

data StaticSHA256 = StaticSHA256 !Bytes32

Is that right? And then get rid of the StaticSize altogether.

@snoyberg
Copy link
Contributor

Yes, though I'd prefer a newtype wrapper in this case. But I'd recommend considering that separate from this PR to avoid slowing it down.

@alexeyzab
Copy link
Collaborator Author

Makes sense. In that case it seems all that's left here is to edit the ChangeLog.md, I'll do that now.

@alexeyzab
Copy link
Collaborator Author

@snoyberg Thanks for walking me through what needed to be done here!

@snoyberg
Copy link
Contributor

No problem, thank you! It looks like the PR is showing all of my changes on extensible-snapshots as differences, which is a bit annoying... anyway, I think this is good to go. @mgsloan, did you have any objections to merge?

@alexeyzab
Copy link
Collaborator Author

alexeyzab commented Jul 14, 2017

I wonder if that's because I merged something the wrong way.

I believe these are the relevant commits for this PR:
1453345
8c87d12
696d9b4
54ff632

Where the last two overwrite some parts of the first two.

UPD: Looks like the CI build timed out.

@snoyberg snoyberg merged commit 54b4fd9 into commercialhaskell:master Jul 17, 2017
@snoyberg
Copy link
Contributor

Thanks!

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.