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

Structured binary #6255

Merged
merged 2 commits into from
Dec 11, 2019
Merged

Structured binary #6255

merged 2 commits into from
Dec 11, 2019

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Sep 24, 2019


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

cabal.project Outdated Show resolved Hide resolved
Copy link
Collaborator

@DanielG DanielG left a comment

Choose a reason for hiding this comment

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

Would be nice if there was some documentation on what exactly this is trying to do. Your commits don't have any messages, pretty hypocritical to complain when other people do this and then not have any yourself :P

From context and digging literally hundres of lines into the diff I'm guessing this is to fix things such as #6163 and #6164 where types of caches with derrived Binary instances change and reading them back either fails or gives the wrong result.

@@ -39,6 +40,10 @@ instance Binary LicenseExceptionId where
then fail "Too large LicenseExceptionId tag"
else return (toEnum (fromIntegral i))

-- note: remember to bump version each time the definition changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to defeat the purpose of this patch. Any way this datatype can be included in the structure hash too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Manual Binary instance => Manual Structured instance.

Yet, here Binary instance is generated by templating, so yes, we should generate Structured instance too. I'll take the license list version and use that.

Good catch.

@phadej
Copy link
Collaborator Author

phadej commented Oct 6, 2019

Would be nice if there was some documentation on what exactly this is trying to do. Your commits don't have any messages, pretty hypocritical to complain when other people do this and then not have any yourself :P

This is WIP PR, I will rewrite commit messages.

@phadej phadej changed the title Structured binary WIP: Structured binary Oct 6, 2019
@phadej phadej added this to the 3.2 milestone Dec 10, 2019
It defines `Structured` type class, which we use to prepend
a hash to cached `Binary` blobs. Thus we can catch early, if
format is changed, avoiding corrupt cache making cabal
behave weirdly.

Plenty types got Typeable instances, as it's a superclass of Structured

This commit also introduces new compat modules:

- Distribution.Compat.Typeable with typeRep
- Distribution.Client.Compat.Orphans,
  to collect at least some orphans into central place.
@phadej phadej changed the title WIP: Structured binary Structured binary Dec 11, 2019
@phadej phadej merged commit 8c3af19 into master Dec 11, 2019
@phadej phadej deleted the structured-binary branch December 11, 2019 12:44
@phadej phadej mentioned this pull request Dec 11, 2019
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.

2 participants