Skip to content

Add support for hpack (see #1679) #1773

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

Merged
merged 1 commit into from
Feb 18, 2016
Merged

Conversation

sol
Copy link
Contributor

@sol sol commented Feb 10, 2016

No description provided.

@sol
Copy link
Contributor Author

sol commented Feb 10, 2016

This adds minimal support for sol/hpack. The required modifications to hpack are not on Hackage yet. I would release a new version of hpack once we are done with code review and ready to merge.

@@ -1062,6 +1064,7 @@ getCabalFileName
=> Path Abs Dir -- ^ package directory
-> m (Path Abs File)
getCabalFileName pkgDir = do
liftIO $ hpack pkgDir
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgsloan I'm not sure if that is the best place to call this. Any feedback?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the right place to call hpack, but this changes the meaning of getCabalFileName enough to warrant renaming it to findOrGenerateCabalFile. The new name isn't perfect (makes it sound like it'll always generate a cabal file if it's missing), however, this is better because it lets the reader know it may perform file system mutations (not merely a "get").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, makes sense. I'll change that some time soon (may have to wait until next weekend).

@mgsloan
Copy link
Contributor

mgsloan commented Feb 17, 2016

Awesome, thanks for working on this! Sorry for letting it languish, I got busy!

In order for package dirtiness to work properly, we need to only write to the cabal file when the hpack file is changed. Maybe rely on modification timestamps and generate a new cabal file only when the hpack is changed?

@phadej
Copy link
Collaborator

phadej commented Feb 17, 2016

@mgsloan AFAIK Hpack.hpack doesn't touch .cabal file (reads but not writes) if nothing changed. That behaviour might need a comment though.

@@ -11,3 +11,10 @@ nix:
- zlib
extra-deps:
- path-io-0.3.1

packages:
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar change is needed for stack-7.8.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, noted.

@sol
Copy link
Contributor Author

sol commented Feb 17, 2016

@mgsloan @phadej Yes, exactly, hpack generates the new cabal file and only writes it to disk if it changed. (see https://github.com/sol/hpack/blob/98110f843e6c981eb3764582295b1926cce198ca/src/Hpack.hs#L66)

@mgsloan
Copy link
Contributor

mgsloan commented Feb 17, 2016

Cool, once the updated hpack is released to hackage, and the comments are addressed, I'd be ready to merge this as an experimental feature.

@sol sol force-pushed the hpack branch 3 times, most recently from f4143d8 to 671a547 Compare February 17, 2016 14:35
@sol
Copy link
Contributor Author

sol commented Feb 17, 2016

@mgsloan I addressed all the open points and released a new version of hpack to Hackage. From my perspective this is good to go now.

@mgsloan
Copy link
Contributor

mgsloan commented Feb 18, 2016

LGTM, thanks!

mgsloan added a commit that referenced this pull request Feb 18, 2016
Add support for hpack (see #1679)
@mgsloan mgsloan merged commit 631e99b into commercialhaskell:master Feb 18, 2016
@sol sol deleted the hpack branch February 18, 2016 02:26
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.

3 participants