Distinguish between component ID and unit ID. #3047

ezyang · 2016-01-15T00:05:06Z

GHC 8.0 is switching the state sponsored way to specify
linker names from -this-package-key to -this-unit-id, so
it behooves us to use the right one. But it didn't make
much sense to pass ComponentIds to a flag named UnitId,
so I went ahead and finished a (planned) refactoring
to distinguish ComponentIds from UnitIds.

At the moment, there is NO difference between a ComponentId
and a UnitId; they are identical. But semantically, a
component ID records what sources/flags we chose (giving us enough
information to typecheck a package), whereas a unit ID records
the component ID as well as how holes were instantiated
(giving us enough information to build it.) MOST code
in the Cabal library wants unit IDs, but there are a few
places (macros and configuration) where we really do
want a component ID.

Some other refactorings that got caught up in here:

- I axed IPI641, fixing #3034.

- Removed some defunct Bacpack fields from ExposedModule
  (GHC doesn't use them.)

- Removed instantiatedWith (also unused) and all of the
  relevant code from configure (it will need to be rewritten
  in any case.)

- Changed the type of componentCompatPackageKey to String, reflecting the
  fact that it's not truly a UnitId or ComponentId.

- Changed the behavior of CURRENT_PACKAGE_KEY to unconditionally
  give the compatibility package key, which is actually what you
  want if you're using it for the template Haskell trick.  I also
  added a CURRENT_COMPONENT_ID macro for the actual component ID,
  which is something that the Cabal test-suite will find useful.

- Added the correct feature test for GHC 8.0 ("Uses unit IDs").

Signed-off-by: Edward Z. Yang ezyang@cs.stanford.edu

23Skidoo · 2016-01-15T00:06:49Z

Do you want this to go into 1.24?

ezyang · 2016-01-15T00:09:29Z

Yes, we'll release GHC 8.0 with it.

ezyang · 2016-01-15T00:19:39Z

Note: Cabal library changes should be good, but I need to work out some details for cabal-install. Will update shortly.

ezyang · 2016-01-15T01:37:53Z

Should be ready.

BardurArantsson · 2016-01-15T06:20:32Z

I started to review this, but realized just how big it is[1]... any chance it could be split up a little bit? (The removal of IPI641 would seem like a reasonable candidate at least.)

I realize that you've probably already tried to, but it can't hurt to ask...

[1] I probably couldn't contribute much in-depth review commentary anyway, but...

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Backpack is going to have to rewrite this functionality. Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

GHC 8.0 is switching the state sponsored way to specify linker names from -this-package-key to -this-unit-id, so it behooves us to use the right one. But it didn't make much sense to pass ComponentIds to a flag named UnitId, so I went ahead and finished a (planned) refactoring to distinguish ComponentIds from UnitIds. At the moment, there is NO difference between a ComponentId and a UnitId; they are identical. But semantically, a component ID records what sources/flags we chose (giving us enough information to typecheck a package), whereas a unit ID records the component ID as well as how holes were instantiated (giving us enough information to build it.) MOST code in the Cabal library wants unit IDs, but there are a few places (macros and configuration) where we really do want a component ID. Some other refactorings that got caught up in here: - Changed the type of componentCompatPackageKey to String, reflecting the fact that it's not truly a UnitId or ComponentId. - Changed the behavior of CURRENT_PACKAGE_KEY to unconditionally give the compatibility package key, which is actually what you want if you're using it for the template Haskell trick. I also added a CURRENT_COMPONENT_ID macro for the actual component ID, which is something that the Cabal test-suite will find useful. - Added the correct feature test for GHC 8.0 ("Uses unit IDs"). Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

ezyang · 2016-01-16T10:19:19Z

I separated out some bits but I'm not sure it helps very much. Not sure if the intermediate commits build.

BardurArantsson · 2016-01-16T11:09:19Z

I'm not sure if this is general policy for this project, but would you mind triggering a build for each of them? Alternatively I suppose it might be possible to just create a PR for each? (I realize that this is annoying, but I guess we'll have to like with it if we want to make sure commits build individually.)

(It can get really annoying when bisecting that a really big commit next to the prime suspect doesn't build.)

BardurArantsson · 2016-01-16T11:14:38Z

... and actually, thinking about it a little more: It might be fine to actually NOT bother trying to build but to just leave this as four commits until review is done and then to re-squash before merge. That way there aren't any weird compatibility effects either, etc.

BardurArantsson · 2016-01-16T11:17:36Z

Well, the three "Remove ..." commits certainly LGTM. I don't have time to review the really big one right now, but will try to do it a little later today.

BardurArantsson · 2016-01-16T11:18:04Z

(And I may not be the best to review this, so I'd encourage others to review too...)

BardurArantsson · 2016-01-16T14:00:17Z

(Mark. End of review)

23Skidoo · 2016-01-16T15:58:51Z

I'm not sure if this is general policy for this project, but would you mind triggering a build for each of them?

We don't have a formal policy, but we've never been strict about this. Bisect allows ignoring broken commits, so personally I'm okay with imposing this cost on the person doing bisect instead of each and every contributor. Don't see any other disadvantages besides slightly worse bisect experience to having potentially broken commits in history.

It might be fine to actually NOT bother trying to build but to just leave this as four commits until review is done and then to re-squash before merge.

Personally I prefer a history composed of smaller self-contained commits instead of large monolithic ones.

Alternatively I suppose it might be possible to just create a PR for each?

Please don't do that, related changes should be grouped together.

BardurArantsson · 2016-01-16T16:00:49Z

OK. I don't necessarily agree, but it's probably not worth fighting over :).

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

BardurArantsson · 2016-01-17T07:19:50Z

Btw, if you use the setup-dev.sh script it should be pretty easy to check if things at least build locally. (There's a caveat in that tests are currently not run if you do this, but they should be built.)

23Skidoo · 2016-01-17T19:13:49Z

LGTM, let's merge.

Distinguish between component ID and unit ID.

ezyang force-pushed the cabal-ghcup branch from 893dffe to fd0360c Compare January 15, 2016 01:37

ezyang mentioned this pull request Jan 15, 2016

Convenience libraries #3022

Merged

ezyang force-pushed the cabal-ghcup branch from fd0360c to 625677d Compare January 15, 2016 18:20

23Skidoo added this to the Cabal-1.24 milestone Jan 15, 2016

ezyang added 4 commits January 16, 2016 01:32

Remove IPI641 compatibility code, fixing haskell#3034.

94e59da

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Remove instantiated-with and related support work.

15e0e71

Backpack is going to have to rewrite this functionality. Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Remove unused field from 'ExposedModule' which GHC does not use.

4e9017f

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

ezyang force-pushed the cabal-ghcup branch from 625677d to ef41f44 Compare January 16, 2016 10:17

ezyang added 3 commits January 16, 2016 09:24

Fix indentation of ghcOptThisUnitId.

b713bf5

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Add mkLegacyUnitId and use it where appropriate.

65e4acd

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Import lint

fb629e3

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

23Skidoo added the cabal-install: other label Jan 16, 2016

23Skidoo added a commit that referenced this pull request Jan 17, 2016

Merge pull request #3047 from ezyang/cabal-ghcup

ecd4760

Distinguish between component ID and unit ID.

23Skidoo merged commit ecd4760 into haskell:master Jan 17, 2016

BardurArantsson mentioned this pull request Jan 21, 2016

Drop 6.4.1 InstalledPackageInfo support #3034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Distinguish between component ID and unit ID. #3047

Distinguish between component ID and unit ID. #3047

Uh oh!

ezyang commented Jan 15, 2016

Uh oh!

23Skidoo commented Jan 15, 2016

Uh oh!

ezyang commented Jan 15, 2016

Uh oh!

ezyang commented Jan 15, 2016

Uh oh!

ezyang commented Jan 15, 2016

Uh oh!

BardurArantsson commented Jan 15, 2016

Uh oh!

ezyang commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

23Skidoo commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 17, 2016

Uh oh!

23Skidoo commented Jan 17, 2016

Uh oh!

Uh oh!

Distinguish between component ID and unit ID. #3047

Distinguish between component ID and unit ID. #3047

Uh oh!

Conversation

ezyang commented Jan 15, 2016

Uh oh!

23Skidoo commented Jan 15, 2016

Uh oh!

ezyang commented Jan 15, 2016

Uh oh!

ezyang commented Jan 15, 2016

Uh oh!

ezyang commented Jan 15, 2016

Uh oh!

BardurArantsson commented Jan 15, 2016

Uh oh!

ezyang commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

23Skidoo commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 16, 2016

Uh oh!

BardurArantsson commented Jan 17, 2016

Uh oh!

23Skidoo commented Jan 17, 2016

Uh oh!

Uh oh!