Add -link-package flag and make -package lazy
Changes PlannedPublic

Authored by niteria on Jun 10 2015, 9:34 AM.

Details

Summary

The -package flag currently does two things:

  • it makes the package avaliable, and
  • it eagerly links the package when running under the interpreter.

When compiling a program using Template Haskell this is unnecessarily
expensive when only a small part of the packages is used for evaluating
TH splices.

This introduces a new flag: -link-package that preserves the current
behavior of -package and changes the behavior of -package so that
it only makes the package avaliable and since the interpreter already
implements lazy loading everything still works.

There are still reasons to want the behavior that -link-package provides, like
C library referring to a symbol from a Haskell package.

Test Plan

harbormaster
manual testing looking at 'Linking' lines in '-v' output for a TH program
I can write a test if anyone feels strongly about it.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
master
Lint
Lint WarningsExcuse: nope
SeverityLocationCodeMessage
Warningcompiler/main/DynFlags.hs:2796TXT3Line Too Long
Warningcompiler/main/DynFlags.hs:3888TXT3Line Too Long
Warningcompiler/main/DynFlags.hs:3891TXT3Line Too Long
Warningcompiler/main/DynFlags.hs:3908TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 7996
Build 9864: GHC Patch Validation (amd64/Linux)
Build 9863: GHC Patch Validation (amd64/Linux)
Build 9862: arc lint + arc unit
niteria updated this revision to Diff 3187.Jun 10 2015, 9:34 AM
niteria retitled this revision from to Add -expose-package flag.
niteria updated this object.
niteria edited the test plan for this revision. (Show Details)
niteria added reviewers: simonmar, austin.
niteria updated the Trac tickets for this revision.
niteria updated this revision to Diff 3188.Jun 10 2015, 9:37 AM

indentation fix

simonmar edited edge metadata.Jun 10 2015, 10:21 AM

Great - but it needs an update to the docs too (docs/users_guide). I suggest adding the two reasons from Trac #2437 for why you might want to use -package rather than -expose-package.

niteria updated this revision to Diff 3189.Jun 10 2015, 10:50 AM
niteria edited edge metadata.

fix ./testsuite/tests/ghc-api/T9595.hs test

austin requested changes to this revision.Jun 11 2015, 11:31 AM
austin edited edge metadata.

Yeah, please drop in user docs, thanks!

This revision now requires changes to proceed.Jun 11 2015, 11:31 AM
austin added a subscriber: ezyang.

Also adding @ezyang for review (since he's been touching a lot of the package infrastructure, I figure he might have a comment or naming bikeshed to bring up)

ezyang edited edge metadata.Jun 11 2015, 12:36 PM

Is there any reason why the default shouldn't be lazy?

Is there any reason why the default shouldn't be lazy?

Good question. Probably not - it might break some rare use cases, but they could be fixed, and changing the default would ensure that Cabal benefits without any further change. @niteria, what do you think?

I can try building stackage with lazy as default. It's hard for me to guess if this breaks anything.

Let's move forward with this. @bgamari maybe we could get this (with -expose-package the default) into 7.12? We'll be doing lots of testing with stackage etc. for 7.12 anyway, so we could make the change and see if any problems show up.

Sounds good to me. I'll do a review tomorrow.

Sounds good to me. I'll do a review tomorrow.

bgamari accepted this revision.Jan 6 2016, 12:57 PM
bgamari added a reviewer: bgamari.

This looks reasonable to me. We can try getting it in to 8.0-rc1.

I still need to update the docs and make lazy the default. I'll try to do that tomorrow.

bgamari commandeered this revision.Jan 6 2016, 1:31 PM
bgamari edited reviewers, added: niteria; removed: bgamari.

I've rebased this.

bgamari updated this revision to Diff 6092.Jan 6 2016, 1:33 PM
bgamari edited edge metadata.

Rebase

bgamari updated this revision to Diff 6093.Jan 6 2016, 1:38 PM
bgamari edited edge metadata.

Use lazy linking by default

bgamari updated this revision to Diff 6094.Jan 6 2016, 1:41 PM

Wibbles

niteria retitled this revision from Add -expose-package flag to Add -link-package flag and make -package lazy.Jan 6 2016, 2:15 PM
niteria updated this object.
niteria commandeered this revision.Jan 6 2016, 3:30 PM
niteria edited reviewers, added: bgamari; removed: niteria.

I'll add some docs.

niteria updated this revision to Diff 6095.Jan 6 2016, 3:30 PM
niteria edited edge metadata.

some docs

bgamari requested changes to this revision.Jan 6 2016, 4:05 PM
bgamari edited edge metadata.

I'm afraid that there are some issues here that will need to be addressed. As the validation might suggest, it seems there is something going awry with the computation of the include paths. All I know is,

  • the GHC build process fails during dependency computation (-M) complaining it cannot find HsVersions.h
  • As far as I can tell, HsVersions.h should be found in compiler/ which is in the include-dirs of the ghc package
  • When this happens preload1 in mkPackageState is empty as one would expect
  • pkgs2 in mkPackageState includes a number of packages including ghc
  • other_flags also includes ghc
  • explicitPackages in the PackageState returned by mkPackageState does not include ghc.
  • it appears that getPackage{Library,Include}Path, which is called by DriverPipeline during cc-like phases, is looking primarily at the preload packages, which seems suspicious
This revision now requires changes to proceed.Jan 6 2016, 4:05 PM

@ezyang, perhaps you would be able to shed some light here?

One little issue I noticed while chasing this down inline.

compiler/main/Packages.hs
1086

I later realized that this should be pkgs1.

ezyang added a comment.Jan 6 2016, 4:37 PM

Oh, this bug makes sense; it sounds like the "preload" set was been overloaded to mean several things; not just linking but also include path computation. I think the correct thing to do is to just separate these, so that -expose-package is accounted for in include path computation, but not linking. Extra include dirs are cheap (unless they conflict) so that should be fine.

(Did you know, Cabal handles this buggily! It's https://github.com/haskell/cabal/issues/2971 but nobody seems to have noticed.)

niteria updated this revision to Diff 6101.Jan 7 2016, 5:20 AM
niteria edited edge metadata.

fix bad rebase

niteria marked an inline comment as done.Jan 7 2016, 5:21 AM
niteria updated this revision to Diff 6117.Jan 8 2016, 9:14 AM
niteria edited edge metadata.

deal with include paths

niteria updated this revision to Diff 6119.Jan 8 2016, 11:03 AM

try to ./validate, compiles on my machine
needs some clean-up

niteria edited the test plan for this revision. (Show Details)Jan 8 2016, 11:14 AM
niteria planned changes to this revision.Jan 8 2016, 12:22 PM

this still fails ./validate

thomie requested changes to this revision.Feb 2 2016, 10:16 AM
thomie added a reviewer: thomie.

This Note in compiler/ghci/Linker.hs should probably get an update:

{- Note [preload packages]

Why do we need to preload packages from the command line?  This is an
explanation copied from #2437:

I tried to implement the suggestion from #3560, thinking it would be
easy, but there are two reasons we link in packages eagerly when they
are mentioned on the command line:

  * So that you can link in extra object files or libraries that
    depend on the packages. e.g. ghc -package foo -lbar where bar is a
    C library that depends on something in foo. So we could link in
    foo eagerly if and only if there are extra C libs or objects to
    link in, but....

  * Haskell code can depend on a C function exported by a package, and
    the normal dependency tracking that TH uses can't know about these
    dependencies. The test ghcilink004 relies on this, for example.

I conclude that we need two -package flags: one that says "this is a
package I want to make available", and one that says "this is a
package I want to link in eagerly". Would that be too complicated for
users?
-}
compiler/main/DynFlags.hs
1161

Comment still mentions -expose-package.

Hi niteria, I closed the Trac ticket this Phabricator revision was previously associated with; I think this Phabricator diff is still a good idea though, because it solves a slightly different problem: while Trac #11244 lets you specify things to be loaded into the interpreter only, this diff lets you specify things to NOT be loaded in the interpreter (-package still is loaded by the interpreter). I think a better ticket for this is Trac #10436

carter added a subscriber: carter.Mar 31 2016, 10:07 PM

Is this one of the changes that is going to land In rc3?

No, it is not an RC-blocker.

Sorry, I meant to ask , will this make it into 8.0?
If my understanding is correct it would make a big difference in build time for th heavy code , right ?

I'm not actively working on this, so I wouldn't expect it for 8.0.
Making the default behavior of -package lazy led to some problems I don't fully understand and having -expose-package was enough to satisfy my use case. I will finish this at some point, but it's not on the top of my priority list.

austin resigned from this revision.Nov 6 2017, 10:09 PM