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

[WIP] Port to cabal-helper-1.0 (git) #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DanielG
Copy link

@DanielG DanielG commented Apr 1, 2019

I'm working on fleshing out the cabal-helper-1.0 API, figured I should see how bad I broke things using the old API so I decided to give porting this project a go. I had to tweak a few things but not too bad overall.

I also fixed compileVersion being too slow (see the discussion here). It only depends on the setup-config header for V1 projects now.

let currentPackageId = HCE.PackageId (T.pack packageName) packageVersion
cabalHelperQueryEnv <- liftIO $ mkQueryEnv (ProjLocV1Dir packageDirectoryAbsPath) (DistDirV1 distDir)
("ghc", packageGhcVersion) <- liftIO $ runQuery compilerVersion cabalHelperQueryEnv
unless (take 2 (versionBranch packageGhcVersion) == take 2 (versionBranch ghcVersion)) $
Copy link
Author

Choose a reason for hiding this comment

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

@alexwl did you think about this? AFAIR GHC can break hi-file format compatibility even with just a minor version bump. So really the versions would have to match exactly.

Copy link
Owner

Choose a reason for hiding this comment

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

It seem that the version of GHC from the header of setup-config contains only two numbers - major and minor (showHeader function from Cabal uses compilerVersion from System.Info: http://hackage.haskell.org/package/base-4.12.0.0/docs/src/System.Info.html#compilerVersion).

Copy link
Author

@DanielG DanielG Apr 2, 2019

Choose a reason for hiding this comment

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

Ah damn, I totally forgot we only get two version components in the header. That won't do at all then, I'll have to revert that compilerVersion optimization then. I think there's just no way around actually going through the helper to get the GHC version in that case.

Honestly that shouldn't be too big a deal though since that will only happen once per Cabal version, regardless of which GHC version is compiling the helper. So the error will take a while longer to pop up but when the user gets to re-trying with a different GHC the helper will already be ready.

Copy link
Author

Choose a reason for hiding this comment

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

OH! Even worse if the header version is from System.Info like you say then this is the GHC version that compiled Setup.hs and not the one Setup.hs decided to use to build the package! Yikes, that's just completely wrong then.

Copy link
Owner

Choose a reason for hiding this comment

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

Does it mean that the correct version of GHC should always be taken from LocalBuildInfo, not from the header of setup-config?

@alexwl
Copy link
Owner

alexwl commented Apr 1, 2019

Thank you Daniel!

It would be great to use the new version of cabal-helper.

I guess I should think about how to index multiple packages at once:

let (packageName, packageVersion) = uiPackageId (NE.head units)
-- ^ in V1 projects there's only one package so this is sound but note
-- this doesn't hold for Stack or V2

Also, thank you for the fixed compilerVersion query.

@DanielG
Copy link
Author

DanielG commented Apr 2, 2019

Yeah, indexing multiple packages would also make things easier for users because you can just let the indexer loose on a whole cabal/stack project rather than just one package :)

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