-
Notifications
You must be signed in to change notification settings - Fork 696
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
Finish new-install #5399
Finish new-install #5399
Conversation
let verbosity' = lessVerbose verbosity | ||
|
||
-- First, we need to learn about what's available to be installed. | ||
localBaseCtx <- establishProjectBaseContext verbosity' cliConfig |
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.
Does this create the dist-newstyle
folder in $PWD
? We should clean up after this in case we are not in a project context if it does.
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.
Oh, no. That doesn't create a fake context, that's creating a real context in the current project... but that breaks out of project installs. Oops.
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.
yeah, I know... I tried real hard to refactor that stuff once. But there is a chicken/egg problem with establishing the project context and file monitor caching in dist-newstyle...
Okay, so this is now feature-complete in the loosest sense. The following cases still need to be handled:
|
|
||
go :: UnitId -> [(ComponentTarget, [TargetSelector])] -> [GhcEnvironmentFileEntry] | ||
go unitId targets | ||
| any hasLib targets = [GhcEnvFilePackageId unitId] |
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.
❯ cat ~/.ghc/x86_64-darwin-8.4.2/environments/default
package-id lnr-1.20.7-359f8114
That's what I get after I try to install linear
. Why do I not get the vowels?
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.
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.
Hmm, how would I get around that...
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.
Not sure that removing the vowels guarantees any shortening of the name, or contributes big savings.
In any case, it is quite alienating to look into .cabal/store
and see that the Grinch stole all the vowels.
I am feeling like I am trolled here, or there was some jester in the team developing the new cabal, and that jester is trying to make fun of the Haskell programmers.
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 filed #7209.
{-# LANGUAGE GeneralizedNewtypeDeriving #-} | ||
{-# LANGUAGE LambdaCase #-} | ||
{-# LANGUAGE RecordWildCards #-} | ||
module Distribution.Client.CmdInstall.EnvironmentParser |
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.
Shouldn't this be in Distribution.Simple.GHC?
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 wasn't sure if it was of general enough applicability, but it's easy enough to move it.
Awww I fucked up the merge with the Github online editor! Sorry about that! |
Just rebase. |
Cabal/Distribution/Simple/GHC.hs
Outdated
@@ -53,10 +53,14 @@ module Distribution.Simple.GHC ( | |||
isDynamic, | |||
getGlobalPackageDB, | |||
pkgRoot, | |||
-- * Constructing GHC environment files | |||
-- * Constructing and deconstsructing GHC environment files |
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.
s/deconstsructing/deconstructing/
Should be done, at least done enough to get smacked around by other people. It works for me™ but I haven't had time/access to machines to test it on anything but macOS. |
cabal-install/cabal-install.cabal
Outdated
@@ -320,6 +320,7 @@ library | |||
zlib >= 0.5.3 && < 0.7, | |||
hackage-security >= 0.5.2.2 && < 0.6, | |||
text >= 1.2.3 && < 1.3, | |||
parsec >= 3.1.13.0 && < 3.2, |
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 see a note in this file: NOTE: when updating build-depends, don't forget to update version regexps in bootstrap.sh
.
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.
Also, is there a less-heavyweight library we could use, to decrease the size of the chicken-or-egg problem here? (Thx for this work, btw!)
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.
Cabal the library already uses parsec anyway.
7.4 is legitimately broken, but it's also outside of our support window. |
It's not working for me:
That GHC it's trying to use is 8.4.2. |
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.
Haven't been able to actually try the command out yet, but here are some notes from looking at the source.
{-# LANGUAGE LambdaCase #-} | ||
{-# LANGUAGE RankNTypes #-} | ||
{-# LANGUAGE RecordWildCards #-} | ||
module Distribution.Simple.GHC.EnvironmentParser |
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.
GHC has code for parsing these files in DynFlags, but this code is better and we should have an eye on reusing it in GHC. Specifically, I think exporting parseEnvironmentFile
along with GhcEnvironmentFileEntry
and its constructors from a non-internal module ought to be enough (which we ought to do anyway, since it's mentioned in the types of this non-internal module).
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.
Already exposed in Distribution.Simple.GHC
. When we get this merged and available to GHC, I'd be happy to write a patch to use this instead. :)
, sdistProjectFile = projectConfigProjectFile (projectConfigShared cliConfig) | ||
} | ||
|
||
sdistAction sdistFlags ["all"] globalFlags |
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.
This feels like the wrong level of interface granularity. The sdist code is going to have to go through the hoops of target parsing, output directory logic, and we're re-wrapping the verbosity in a flag. Can CmdSdist export a more convenient function for this?
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.
It even has that, just about, and I'm not at all sure why I'm using the sdistAction
.
@@ -130,27 +172,188 @@ installAction (configFlags, configExFlags, installFlags, haddockFlags) | |||
die' verbosity $ "--enable-benchmarks was specified, but benchmarks can't " | |||
++ "be enabled in a remote package" | |||
|
|||
-- We need a place to put a temporary dist directory | |||
let |
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.
In GitHub's infinite wisdom I can't attach review comments to lines that aren't changed, but there's a comment above with some TODOs. Those are solved now, right?
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.
Yep.
GhcEnvFileClearPackageDbStack | ||
: fmap GhcEnvFilePackageDb packageDbs | ||
entries = baseEntries ++ entriesForLibraryComponents (targetsMap buildCtx) | ||
createDirectoryIfMissing True (takeDirectory envFile) |
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.
If we're bothering to avoid creating .environment
files on old GHCs, shouldn't the above lines be under the when
?
@@ -199,13 +402,37 @@ installAction (configFlags, configExFlags, installFlags, haddockFlags) | |||
traverse_ (symlinkBuiltPackage verbosity mkPkgBinDir symlinkBindir) | |||
$ Map.toList $ targetsMap buildCtx | |||
runProjectPostBuildPhase verbosity baseCtx buildCtx buildOutcomes | |||
|
|||
unless supportsPkgEnvFiles $ | |||
warn verbosity "The current compiler doesn't support safely installing libraries. (GHC 8.0+ only)" |
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.
Since this means that libraries aren't installed, I think it should say that rather than mentioning 'safely' here.
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'm intending it as "doesn't support doing it safely (so we're not doing it at all)". Could be clearer though.
sdistAction sdistFlags ["all"] globalFlags | ||
|
||
(specs, selectors) <- withInstallPlan verbosity' localBaseCtx $ \elaboratedPlan -> do | ||
-- Split into known targets and hackage packages. |
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.
The code under here is really deeply nested. Can we consider lifting some of this out, or are there so many local variables involved that it's annoying?
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.
There's heavy enough cross-involvement that the drop in complexity for lifting bits out is more than made up for by the increase from having to directly plumb everything. I wrote it split out originally but I was passing the same arguments over and over often enough I manually inlined it into that code there.
Cabal/doc/nix-local-build.rst
Outdated
``cabal new-install [FLAGS] PACKAGES`` builds the specified nonlocal packages | ||
and symlinks their executables in ``symlink-bindir`` (usually ``~/.cabal/bin``). | ||
``cabal new-install [FLAGS] PACKAGES`` builds the specified packages, adds their | ||
libraries into the default environment and symlinks their executables in |
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.
Should probably link to the GHC manual describing package environments.
I think we also need to describe the practical effects of this. The GHC docs say this:
Note the implicit -hide-all-packages and the fact that it is -package-id ⟨unit-id⟩, not -package ⟨pkg⟩. This is because the environment specifies precisely which packages should be visible.
So if I don't have a default package environment before, then I cabal new-install
something, and now every manual GHC invocation will have an implicit -hide-all-packages
. I am not sure this is going to make users happy.
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.
Well, that's only a real issue if you are using both install commands at once. I'm personally comfortable saying "don't do that, then", but that's just me.
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.
IIUC this will also apply to, e.g., packages installed to the global store using the system package manager as well. I'd be okay with saying that v1-install
and new-install
don't play nicely together but I'm more hesitant about that interaction.
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.
Hmm... We could just include all of the global packages.
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.
My personal opinion is that the package manager's packages are for system dependencies not for building software locally.
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.
Thinking further, exposing package manager packages is an emphatic Bad Choice, as far as I'm concerned. That greatly expands the burden in terms of creating a consistent install plan, and will likely end up with a lot of situations where you are stuck on old version because you need some random (and likely antique) version of text
or something because that's what the package manager provides.
envEntries <- if | ||
| compilerFlavor == GHC || compilerFlavor == GHCJS | ||
, supportsPkgEnvFiles | ||
, envFileExists -> readGhcEnvironmentFile envFile |
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.
If a failure to install libraries isn't fatal because GHC is old, then I think we should gracefully handle an extant-but-unparseable default package environment by warning about that and then installing the executables anyway.
createDirectoryIfMissing True (takeDirectory envFile) | ||
when supportsPkgEnvFiles $ do | ||
let | ||
entries' = nub (envEntries ++ entries) |
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.
First, is this nub
absolutely never going to go wrong and lead to a non-Cabal-generated package environment being destroyed, including, e.g., making it easy to roll back Cabal's changes?
Second, a user might install many thousands of packages. ordNub
is O(n log n), so go for that instead.
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.
This code generally considers that env as belonging to Cabal, but that isn't explicit. Either needs the code to change or the docs to call out that we own that file.
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.
A mention in the docs that Cabal considers this file its property ought to suffice.
configCompilerEx hcFlavor hcPath hcPkg progDb verbosity | ||
|
||
let | ||
envFile = home </> ".ghc" </> ghcPlatformAndVersionString platform compilerVersion |
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.
If new-install
grows a --target-environment
flag that takes a file naming the environment to write to, rather than always being the default package environment, then I think testing this code gets a lot simpler.
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.
Agreed.
Should have fixed your issue, @quasicomputational. |
Why is there a parse error only on 7.10? |
Sadly not, still happening with 0b2817f (this PR's latest commit). |
For some reason, it doesn't have your compiler in the program db it passes to |
No clue here, sorry. I can provide diagnostics if you've got any idea what might be helpful, or any new logging you put in to try and work out what's going on. |
I'll work on it more seriously after breakfast. 😝 |
on line CmdInstall.hs:319 would be helpful. |
|
@tom-bop, it's the sum of existing libraries ( |
@typedrat 👍 , but it won't for example have any of the package you " |
@tom-bop, that's correct. |
This is a good question. Whether or not we do it, it would be good to bikeshed what the "right" name might be, for conceptual clarity if nothing else. I was thinking "provide"? |
Or |
I landed here wondering why While I am sure I don't fully understand the discussion, there's one important point I haven't seen mentioned. So here's my drive-by comment. even when 3.0 lands, there will still be heaps of references on the internet to
I'd say go for it then. Thanks. |
If we're going to add |
@ekmett, it's less trivial than it seems to add aliases, and I feel like the likelihood of a third new paradigm is low enough to not merit the work to add them for |
Then I don't particularly look forward to enduring all the same complaints when we get around to a v3. ;) |
Y'know what, it's not as bad as I thought, but that'd be a different PR. :) |
@ekmett Well, my idea was rather to consider
|
@hvr You have to admit that state of affairs seems messy and inconsistent, especially in the event we ever do ship a At some unknown future point we may or may not get around to a But in any of those worlds, having What I'd ideally want is to be able to make one change to my build scripts to migrate to |
@ekmett yes, in hindsight, having used Also, if we decide to introduce
So the trade-off here is, either
In hindsight, we should have introduced |
Is there a cost to simply having a That way we'd have
|
Closes #4558.
Adds support for:
--
Please include the following checklist in your PR:
[ci skip]
is used to avoid triggering the build bots.