-
Notifications
You must be signed in to change notification settings - Fork 29
Fixes #94 #105
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
Fixes #94 #105
Conversation
66ab794
to
b41e985
Compare
CI failure is due to too old stack |
src/Stack2nix/External/Cabal2nix.hs
Outdated
import Text.PrettyPrint.HughesPJClass (Doc) | ||
|
||
cabal2nix :: Args -> FilePath -> Maybe Text -> Maybe FilePath -> DB.HackageDB -> IO (Either Doc Derivation) | ||
cabal2nix Args{..} uri commit subpath hackageDB = do | ||
let runCmdArgs = args $ fromMaybe "." subpath | ||
hPutStrLn stderr $ unwords ("+ cabal2nix":runCmdArgs) | ||
cabal2nixWithDB hackageDB runCmdArgs | ||
options <- parseArgs runCmdArgs | ||
cabal2nixWithDB hackageDB (options { optNixpkgsIdentifier = \i -> Just (binding # (i, path # ["pkgs", i])) }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of using attr
it will choose pkgs.attr
?
Could this update of options be put into a separate function just to make it easier to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure :)
parsePackageData :: PackageName -> U.PackageData -> P.PackageData | ||
parsePackageData pn (U.PackageData _ vs') = | ||
mapException (\e -> HackageDBPackageName pn (e :: SomeException)) $ | ||
Map.mapWithKey (P.parseVersionData pn) $ vs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so deprecated package versions are noted in the preferred version range. And this function doesn't do filtering of packages by version range. Could the lack of any version range check cause problems or is it ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See NixOS/hackage-db#7 for context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK got it I think. Any preferred version ranges or deprecated versions added by package authors have the potential to cause build failures if these packages are in a stackage LTS.
Fixed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great :-)
Once this and #95 are merged, I'll prepare changelog and we can do 0.2 release. |
I know you guys are extremely busy so here's my proposal to resolve the feedback loop. If no decision is made from IOHK, I'll do option c (as my only option) on 23rd July. a) Grant me commit access (and possibly hackage maintainer access) as co-maintainer of stack2nix b) I'm willing to be the core maintainer of stack2nix and stack2nix repository is transfered to me (with hackage ownership) and I'll make sure your PRs are reviewed - this means less risk+work for IOHK. c) Fork stack2nix, rename it and release it as a fork (I'd like to avoid that as much as possible in favor of a or b) |
Hi @domenkozar , We intended to give you commit access so that you could merge this, but I think in the process someone accidentally revoked their own admin access and hasn't been able to get it back yet. |
Oh that's unfortunate. Thank you guys <3 |
@domenkozar, you are now an admin collaborator on the repo. Thank you for volunteering to help. |
cc @jmitchell @angerman @rvl
This should prepare stack2nix for a release compatible with LTS-12, as soon as cabal2nix is released.