Details

Reviewers

simonmar
ezyang
bgamari
thomie
austin

Trac Issues

#2437

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

arcpatch-D973

Lint

Lint Warnings

Excuse: meh

Severity	Location	Code	Message
Warning	compiler/main/DynFlags.hs:2743	TXT3	Line Too Long
Warning	compiler/main/DynFlags.hs:3834	TXT3	Line Too Long
Warning	compiler/main/DynFlags.hs:3837	TXT3	Line Too Long
Warning	compiler/main/DynFlags.hs:3854	TXT3	Line Too Long

Unit

No Unit Test Coverage

Build Status

Buildable 7997
Build 9866: GHC Patch Validation (amd64/Linux)
Build 9865: 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.

Herald added subscribers: bgamari, thomie. · View Herald TranscriptJun 10 2015, 9:34 AM

indentation fix

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.

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

Yeah, please drop in user docs, thanks!

This revision now requires changes to proceed.Jun 11 2015, 11:31 AM

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)

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.

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.

I've rebased this.

Rebase

Use lazy linking by default

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.

I'll add some docs.

Herald edited edge metadata. · View Herald TranscriptJan 6 2016, 3:30 PM

some docs

Herald edited edge metadata. · View Herald TranscriptJan 6 2016, 3:30 PM

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`.

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.)

fix bad rebase

deal with include paths

It fails with https://phabricator.haskell.org/P82 now

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

this still fails ./validate

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
1108	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

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

watashi added a subscriber: watashi.Nov 11 2018, 5:46 PM

			Path	Packages
M			compiler/main/DynFlags.hs (45 lines)
M			compiler/main/Packages.hs (12 lines)
M			docs/users_guide/packages.rst (15 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	3187	a48167e		Jun 10 2015, 9:34 AM	★	★
Diff 2	3188	a48167e	indentation fix	Jun 10 2015, 9:37 AM	★	★
Diff 3	3189	a48167e	fix ./testsuite/tests/ghc-api/T9595.hs test	Jun 10 2015, 10:49 AM	★	★
Diff 4	6092	7861a22	Rebase	Jan 6 2016, 1:33 PM	★	★
Diff 5	6093	7861a22	Use lazy linking by default	Jan 6 2016, 1:38 PM	★	★
Diff 6	6094	7861a22	Wibbles	Jan 6 2016, 1:41 PM	★	★
Diff 7	6095	89ba83d	some docs	Jan 6 2016, 3:30 PM	★	★
Diff 8	6101	89ba83d	fix bad rebase	Jan 7 2016, 5:20 AM	★	★
Diff 9	6117	89ba83d	deal with include paths	Jan 8 2016, 9:14 AM	★	★
Diff 10	6119	89ba83d	try to ./validate, compiles on my machine	Jan 8 2016, 11:03 AM	★	★

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

Details

Diff Detail

Revision Contents
Changeset List

Diff 6095

compiler/main/DynFlags.hs

compiler/main/Packages.hs

docs/users_guide/packages.rst

Add -link-package flag and make -package lazyChanges PlannedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 6095

compiler/main/DynFlags.hs

compiler/main/Packages.hs

docs/users_guide/packages.rst

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

Revision Contents
Changeset List