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

Cabal lib for nix local build #2948

Merged
merged 28 commits into from Dec 16, 2015

Conversation

Projects
None yet
4 participants
@dcoutts
Copy link
Member

dcoutts commented Dec 16, 2015

@23Skidoo @hvr ping

A series of patches to the Cabal lib that are needed for the nix-local-build branch of cabal-install.

There's quite a few patches here because I've tried to make each one be clear and do one thing.

Summary:

  • Misc exports of useful functions
  • Extra instances
  • Change the interface to registerPackage so we can use it in cabal-install. This allows cabal-install to register things itself, rather than always relying on Setup register. This requires that registerPackage take fewer unnecessary or big arguments (like LocalBuildInfo).
  • Extend registerPackage with a multi-instance capability
  • Add compiler-independent utils for creating, deleting and testing existence of package dbs, including requesting using dir-style, but keeping the ability for Cabal lib to use file style in preference
  • Expose enough info to be able to do file change monitoring, for package dbs and for programs. This just means reporting file locations of things, so that cabal-install can monitor those files for changes.

dcoutts added some commits Dec 14, 2015

Add more standard derived instances, Eq, Generic and Binary
Most types have these already. This just adds a few more.
UHC: factor out getGlobalPackageDir function
It's used three times already. This isn't important on it's own, but
simplifies subsequent changes, when we add yet another use of it.
UHC: registerPackage stop using source pkg and inplace args
UHC's version of registerPackage is the only one that makes use of the
PackageDescription and inplace :: Bool args, and it's quite wrong for
doing so. Registering a package should depend on the content of the
InstalledPackageInfo and the PackageDBStack to register into and the
Compiler to register with. It should not depend on the original source
PackageDescription, and should not need a separate inplace arg. The
location is determined by the PackageDBStack. UHC was not following
this pattern and thereby forcing the general compiler independent
registerPackage to take annoying and unnecessary arguments.

With this patch, the register location is determined by the
PackageDBStack. The source package id also comes from the
InstalledPackageInfo rather than the source PackageDescription.

This patch does not yet change the registerPackage type.
Add and export a showProfDetailLevel utlity
To be used in cabal-install. Also use it in one place in Cabal.
Change the interface to the per-compiler registerPackage functions
Remove the now-unused PackageDescription and inplace :: Bool args.

Not yet changed the compiler-independent registerPackage.
Move user logging out of registerPackage and into some callers
The main reason is to stop using the pkg and inplace args so that we
can drop them entirely. A side benefit is that we don't actually want
to emit a setupMessage for inplace registering, since that's a rather
uninteresting internal action. We only want it for the explicit
register command. So only one caller gains a call to setupMessage.
Don't pass LocalBuildInfo to per-compiler registerPackage functions
Rather, pass the individual bits we need, which is the program db
and in some cases the compiler. This is a step towards having the main
registerPackage not take the LocalBuildInfo. That is useful in contexts
like cabal-install where we do not have a full LocalBuildInfo, but we
still want to be able to register packages in a compiler-agnostic way.
Change regiserPackage to not require so much info
Drop the now-unused PackageDescription and inplace :: Bool args. And
instead of taking the whole LocalBuildInfo, just take the bits we need:
the compiler and program db. The package db stack was already passed in
separately. Also reorder args to follow standard conventions.
Extend HcPkg.init support for dir-style package dbs
The HcPkgInfo useSingleFileDb is split into two: supportsDirDbs and
requiresDirDbs. Then rather than HcPkg.init callers having to do the
writeFile [] thing, HcPkg.init does it itself automatically based on the
HcPkgInfo. In the case that supportsDirDbs is True but requiresDirDbs is
False then we have a choice, to use dir style or file style. For
compatability reasons, when using ghc/ghc-pkg for the inplace package db
we want to use the old file style, even though dir style is supported.
However in other circumstances (e.g. in places in cabal-install) we
would like to use the dir style if it's supported, and there are no
backwards compat issues. So HcPkg.init gains a new Bool arg to request
using the file style if it's still supported. Only this mode is used
within Cabal itself, but the non-compat mode is available for other
users.

The compiler-independent initPackageDB is left with the same old
behaviour, but a new createPackageDB has the extra compat argument
(which is only passed to hc-pkg for ghc-pkg).
Add new compiler independent package db utils
Add doesPackageDBExist and deletePackageDB, and export the new
createPackageDB. This gives a more complete compiler-independent API for
package db manipulation.
Use the new package db utils to simplify createInternalPackageDB
And fix up the db path for UHC. UHC cannot just register anywhere, like
compilers that have a hc-pkg can. It has to be special locations. Now
that registerPackage no longer takes the inplace :: Bool arg, UHC's impl
of registerPackage has to get the dir from the PackageDbStack without
knowing if it's implace or not. So the correct inplace location has to
be set earlier for the inplace package db, which is what this does.
Factor impl of registerInvocation' slightly
Prior to further extension. Just share the common args calculation.
Extend HcPkgInfo for mulit-package instances capabilities
Part 1: just add the fields and fill them in for each HcPkg user.
Add new HcPkg.registerMultiInstance
This supports the feature of newer ghc-pkg version which have the
register --enable-multi-instance flag. This allows registering multiple
instances of the same version of a package into a single database.

In addition, to support the same feature on some older ghc-pkg versions,
the HcPkgInfo has a recacheMultiInstance capability, which tells us if
the trick of registering multiple instances by running ghc-pkg recache
works. This is known to work for all versions of ghc-pkg that support
the recache command at all.

Then HcPkg.registerMultiInstance will use one of the two methods
depending on which is supported, or fail if neither is. Currently only
registering into specific package dbs is supported, not global or user.

This new multi-instance feature is needed for cabal-install.
Add mutli-instance support to registerPackage
With support for GHC and GHCJS. All Cabal lib internal uses remain
traditional single instance, so there's no change of behaviour.
Add new getInstalledPackagesMonitorFiles
It provides a way to find out what files need to be monitored to detect
changes in a compiler's package database.

This is not used within the Cabal lib.
Be consistent about using findProgramOnSearchPath
The findProgramOnSearchPath function is what we use in most places to
implement the Program abstraction's programFindLocation method.
Re-export it via Program module.

The only place that was still using the old findProgramLocation
instead was in HaskellSuite. Deprecate findProgramLocation which
is now no longer used.

This is in preparation for changing the return type of
findProgramOnSearchPath.
Generalise findProgramOnSearchPath to calculate locations looked in
But in this patch, don't actually return them yet, so not yet changing
the resturn type.

The purpose here is change monitoring. In cabal-install we want to be
able to automatically re-run various actions when the build environment
changes, including when programs have changed (e.g. due to a $PATH
change, finding the compiler in a different location). The basic
information required for such change monitoring is not only the location
where a program was ultimately found, but all the locations where it was
looked for and not found. Otherwise one will miss the case where having
previously found the program in one location, later on the program
appears earlier in the search path. In this case a build system should
notice and react, but if it only monitors the ultimate location where
the program was found then this is impossible. The build system also
needs to monitor the locations where the program was not found, to make
sure it is still not there. This principle actually applies anytime we
have a file search (e.g. hs-source-dirs), programs are just one example.
Extend ConfiguredProgram with search locations looked at
This is to allow monitoring programs for changes. The combination of the
location where the program was found, and all the other locations where
the program was looked for gives the full set of files to monitor for
changes in that program.

The Program programFindLocation method is extended to return the
locations looked at but where the prog was not found. The default
implementation of programFindLocation, findProgramOnSearchPath, is
extended to return those locations.

Other places have to change, mostly just the type. In a couple places in
GHC & GHCJS where there is additional searching done, the not-found
locations have to be collected and returned.
Adjust instance Binary ProgramDb to include the ProgramSearchPath
The ProgramDb Binary instance is a bit odd by leaving out the Programs,
only including the ConfiguredPrograms. Of course this is because the
Programs contain functions. But the ProgramSearchPath is concrete and
should be included.
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Dec 16, 2015

@dcoutts Thanks, will take a look later today. Can you fix the Travis failure?

dcoutts added some commits Dec 16, 2015

Further CPP fix
For some reason my cpp does not fail with #if THING_NOT_DEFINED
but the travis one does, so use #ifdef instead.
@hvr

This comment has been minimized.

Copy link

hvr commented on Cabal/Distribution/Simple/Program/Find.hs in adcfe14 Dec 16, 2015

I recommend using #define FOO 1, as that matches what -DFOO on the CLI maps to, and if you happen to write #if rather than #ifdef stuff still works

Long-term, I propose to centralise such #define in a single shared .h file. And once GHC 8.0 becomes the oldest supported GHC, we can just assume MIN_VERSION_...() macros to be always available.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 89fd3e0 Dec 16, 2015

OK. (Do we have any sense for how much these derived instances increase compile time? I feel like it is nontrivial.)

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

We have these instances for most other types in these modules already, and we do actually use them. The ones we could probably omit these days is the Read and Data instances.

This comment has been minimized.

Copy link

ezyang replied Dec 16, 2015

I know we can't get rid of them. But I marvel at how long it takes Cabal to build at -O, I suspect deriving has to do with it (since many of the slowest modules are defining big datatypes with lots of deriving) and I wonder if there isn't anything we can do about it. Not anything for this patchset, of course.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 7d01c2e Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 0501e50 Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on f1ca968 Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 27dd76a Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on b0b57da Dec 16, 2015

OK. (it's a bit annoying that UHC needs comp but whatever.)

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

In principle they all ought to be given their Compiler value, and not be given the ProgramDB. So I don't see UHC as unreasonable here (at least not now that I stopped it looking at the PackageDescription and inplace :: Bool).

This comment has been minimized.

Copy link

ezyang replied Dec 16, 2015

OK, fine by me.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on dee7e0a Dec 16, 2015

OK. Typo in commit message (regiser).

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 12b222a Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on ea6dabe Dec 16, 2015

OK. (BTW, Cabal has always had this bug, but removeDirectoryRecursive follows symlinks. We probably shouldn't use it.)

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

I'm not completely sure I know what you're talking about here.

This comment has been minimized.

Copy link

ezyang replied Dec 16, 2015

Oops, I'm describing pre-1.2.2.0 directory behavior. (Which is in our support window, but I suppose is fading fast.)

@ezyang

This comment has been minimized.

Copy link

ezyang commented on e86851d Dec 16, 2015

OK. (This has all been tested on UHC? Setting up the builder with UHC would be nice.)

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

I'm afraid it has not. I've just tried to be careful to not actually change behaviour. I don't think it's critical for a release that goes along with GHC for it to be tested with UHC, we can pick up fixes for them for later releases.

This comment has been minimized.

Copy link

ezyang replied Dec 16, 2015

OK. (I still think we should drop support for the other compilers, but maybe that's just me :) haskell#2714

@ezyang

This comment has been minimized.

Copy link

ezyang commented on b54f36a Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 728fdcf Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on ae78ae0 Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 7b4df7e Dec 16, 2015

There needs to be a comment explaining what the "recache" trick is, in the source code.

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

Will do.

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

Done.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 2d0a878 Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 3ff32fc Dec 16, 2015

OK.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 7e409fc Dec 16, 2015

Test? (No GHCJS support?)

I don't understand the getUserPackageDB implementation. Is this path really not calculated by Cabal today? (Is it code in cabal-install? Why wasn't it moved over?)

Do you actually ever return a directory? I don't think so, because for directory package DBs you just look at the binary cache file, right? (Maybe a remark that this function only picks up changes if you RECACHE).

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

No GHCJS support yet, but should be similar. I've not tested GHCJS support in the new cabal-install branch either yet.

Re getUserPackageDB, no, currently neither Cabal nor cabal-install has to calculate the actual location of the user package db, it's always referred to by name, which translates into --user etc flags to ghc/ghc-pkg. Here to do the change tracking we must pierce that state hiding that ghc is doing.

Re recache, that's quite true, which matches up exactly with when ghc itself will notice the changes. Can certainly add a comment to this effect.

This comment has been minimized.

Copy link

ezyang replied Dec 16, 2015

It's a bit distressing we can't interrogate GHC for this info. And indeed, I don't see any way to get versionedAppDir or resolvePackageConfig from GHC on the command line. I'll submit a PR to mention that these are reimplemented in Cabal.

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

Thanks. Putting all these things into --info would be best, minimises the number of calls we have to make to ghc when configuring. It's there in the --info for the global package db. Of course we'll still need this annoying code for older ghc, but at least at that point we know that they're not going to change again in future.

This comment has been minimized.

Copy link

23Skidoo replied Dec 16, 2015

/cc @luite

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 7d4fbb2 Dec 16, 2015

OK.

Add a comment to explain the HcPkg.recache trick
For supporting multi instance registrations on older GHC versions.
@ezyang

This comment has been minimized.

Copy link

ezyang commented on ee0ed0b Dec 16, 2015

Typo: resturn/return

I don't think this should stop this patch from going in, but I generally have a poor opinion of manually remembering what places you have probed. Way easy to forget to that you probed a location, the resulting bug is difficult to find (some strange nondeterminism) and you're constantly playing whackamole.

Here's an alternate approach: make a new monad for tracking file dependencies, don't export a MonadIO instance, reimplement System.Directory with mandatory tracking. (Or maybe some people would say you should just instrument syscalls...)

This comment has been minimized.

Copy link
Owner

dcoutts replied Dec 16, 2015

The latter is essentially the approach we do take in cabal-install in the new branch. However, converting everything over in Cabal lib as well right now is a bit much. But I agree we ought to have a build monad at some point in Cabal, to take care of several things, including this.

@ezyang

This comment has been minimized.

Copy link

ezyang commented on b39b906 Dec 16, 2015

OK (modulo comment on previous commit.)

@ezyang

This comment has been minimized.

Copy link

ezyang commented on 96a010a Dec 16, 2015

If it's not too much effort, a comment explaining why having functions in Programs is unavoidable, and why it's fine for ProgramDb to leave them out, would be nice. Mostly hoping you looked at this, figured out why that was the case, and can write it down.

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Dec 16, 2015

Let's ship it!

dcoutts added some commits Dec 16, 2015

Add a few comments explaining implementation assumptions
About the ghc package dbs in particular.
@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Dec 16, 2015

So I think I've addressed everything except for tests for getUserPackageDB, and adding an impl of getInstalledPackagesMonitorFiles for GHCJS. I hope neither of these are blockers however.

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Dec 16, 2015

I wouldn't block on it!

@23Skidoo

This comment has been minimized.

s/ths/this/

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Dec 16, 2015

LGTM as well, let's merge!

@hvr

This comment has been minimized.

Copy link
Member

hvr commented Dec 16, 2015

👍

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Dec 16, 2015

Woo!

dcoutts added a commit that referenced this pull request Dec 16, 2015

@dcoutts dcoutts merged commit d71de1d into haskell:master Dec 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment