-
Notifications
You must be signed in to change notification settings - Fork 710
Fix monolithic inplace build tool PATH #5633
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
Conversation
497db22
to
d5781dc
Compare
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "497db229ca68243e7c8e9c03831d529a99d38882", "tag":"linux-7.10.3" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "497db229ca68243e7c8e9c03831d529a99d38882", "tag":"linux-7.8.4" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "497db229ca68243e7c8e9c03831d529a99d38882", "tag":"linux-8.0.2" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "497db229ca68243e7c8e9c03831d529a99d38882", "tag":"linux-7.6.3" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "497db229ca68243e7c8e9c03831d529a99d38882", "tag":"linux-8.2.2" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"linux-7.10.3" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"linux-8.0.2" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"linux-7.8.4" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"linux-7.6.3" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"linux-8.2.2" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"linux-8.4.4" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"linux-8.4.4-fdebug-expensive-assertions" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"osx-7.8.4" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"osx-7.10.3" }
I can confirm this fixes the original repro-case from #5104; I'm currently dogfooding this PR locally |
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "d5781dc7b857a3d802821d6be958134f086dff60", "tag":"osx-8.0.2" }
@23Skidoo fwiw, I couldn't find any empirical problems yet; I'd suggest merging |
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.
LGTM modulo minor stylistic issues.
, Just path <- [planPackageExePath pkg] ] | ||
exe_map1 = Map.union external_exe_map exe_map | ||
, let paths = planPackageExePaths pkg ] | ||
exe_map1 = Map.union external_exe_map (pure <$> exe_map) |
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.
I'd rather be a bit more explicit here and write this as M.map singleton exe_map
(if there's no singleton :: a -> [a]
in cabal-install
, we can add it to Distribution.Compat.Prelude
or a utils module. Or just use (\a -> [a])
.
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.
What about singleton <$> exe_map
or fmap singleton exe_map
? It's clear that it's a map from the Map.union
imo
binDirectoryFor | ||
distDirLayout | ||
elaboratedSharedConfig | ||
elab $ | ||
elab <$> |
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.
I'd use map
instead of <$>
here as well.
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.
Like map (binDirectoryFor ...) case ...
? or inline map
? The first is a bit hard to follow imo, and inline map is just <$>
(it's clear that we are acting on a []
from the type sig)... maybe a comment instead?
Or what about a binding?
elab <$> | |
elab | |
<$> executables | |
where | |
executables = |
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.
I was thinking of `map`
instead of <$>
.
@fgaz Both suggestions sound fine, use your discretion. |
Monolithic inplace packages have a directory for each exe, so we allow multiple paths and return all the exe paths of the package. Fixes haskell#5104
d5781dc
to
c81bc98
Compare
Cherry picked on 2.4 |
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "c81bc98faecd922da640455f99c10008ff3fb191", "tag":"linux-7.10.3" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "c81bc98faecd922da640455f99c10008ff3fb191", "tag":"linux-8.0.2" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "c81bc98faecd922da640455f99c10008ff3fb191", "tag":"linux-7.8.4" }
"url":"pull/5633", "account":"haskell", "repo":"cabal", "commit": "c81bc98faecd922da640455f99c10008ff3fb191", "tag":"linux-7.6.3" }
Monolithic inplace packages have a directory for each exe, so we allow
multiple paths and return all the exe paths of the package.
Fixes #5104
/cc @hvr