Clarify pkg selection when multiple versions are available
ClosedPublic

Authored by harendra on Aug 22 2016, 9:45 AM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
harendra updated this revision to Diff 8453.Aug 22 2016, 9:45 AM
harendra retitled this revision from to Clarify pkg selection when multiple versions are available.
harendra updated this object.
harendra edited the test plan for this revision. (Show Details)
harendra added a reviewer: ezyang.
harendra updated this revision to Diff 8454.Aug 22 2016, 2:04 PM
harendra edited edge metadata.

Adding a few more commits related to package databases' documentation

  • Update the documentation on package databases
  • Move "package environments" section
  • Retain a space between a flag and its argument
ezyang edited edge metadata.Aug 22 2016, 3:13 PM

Thanks for the doc patch, much appreciated.

As a minor tooling note; if you have logically distinct patches, it's best to arc diff them separately, so they don't all get smooshed into one diff. Not a big deal here (since it's pretty obvious what the separate changes are) but worth noting.

docs/users_guide/packages.rst
151

This is only true when -hide-all-packages is not specified.

386

Forward reference here? Also the augmenting behavior doesn't seem so useful to mention here.

388

I guess, an example here would be useful?

390

Term of art is installed package id (foo-0.1-xxx), not package id (foo-0.1)

393

There's more to shadowing than just that.

Suppose that q-0.1-aaa depends on p-0.1-aaa in some database. If you then put another db on top that redefines p-0.1-aaa, whether or not q-0.1-aaa is now broken depends on the ABIs of the packages (as recorded in the abi field in the package database.

If the ABIs match, q-0.1-aaa remains unbroken, and points to the NEW p (since they were ABI compatible). If the ABIs don't match, then we remove q-0.1-aaa as a broken package.

400

will choose an unspecified instance of the package.

448–453

maybe, specifies a stack of databases from top to bottom?

450

(where entries earlier in the PATH override ones that are later.)

454–455

The older word was clearer, I think. The point is that a trailing : is an alias to add the user package db and then the global package db.

Please pardon my ignorance. This was the first time I changed anything in GHC code base or used arc/phabricator. I hoped it will show separate commits like github. But I realized only after uploading that it shows only the combined diff. I have a related question, when I make more changes and upload to the same diff, what happens to the previous review/comment history, is it retained by the tool? Does it get affected in any way?

I responded to most of your comments, I agree to those that I did not address explicitly. I will rework and upload again.

docs/users_guide/packages.rst
151

When -hide-all-packages is specified this should not matter since all packages are anyway hidden already. Is there something I am missing? I found it easier to comprehend without stating that condition.

386

I think I should merge this and the previous paragraph into one. They both seem to be conveying redundant information. I will incorporate these comments as well in that and then you can review again.

388

I seem to be too lazy for that :-) What should it be? A command run involving GHC_PACKAGE_PATH and -package-db ? Here or after the commands?

390

Ok. I will correct all references.

393

Ah, ok. This is not documented anywhere I guess so we must add this too.

In other words, we always use the one on top and the references to the same version in lower layers are "de-duplicated" if the ABI's match or removed if they don't.

Isn't this a generalization of "Version conflict resolution" as described in the next para? And this does not seem to be consistent with what we are saying there because we are in fact choosing the top one. Or are we saying that behavior is unspecified when no other packages depend on it?

In fact we should document the impact of package selection and dependencies in general. For example choosing a particular version of a package means all packages which depend on the versions not chosen will now become unavailable.

454–455

Ok, got your point. I think I should rephrase it so that there is no ambiguity.

harendra added inline comments.Aug 23 2016, 8:27 AM
docs/users_guide/packages.rst
393

Perhaps, I misunderstood what you said. I guess what you said applies only when there is an IPID conflict and not applicable for version conflict. So there is no inconsistency.

harendra added inline comments.Aug 23 2016, 9:13 AM
docs/users_guide/packages.rst
393

Even the last statement I made in first response above about other versions not being available is not true in general because that's how we ended up in the stack bug with two different versions of the same package. Though it is true when -package is used.

Why don't we do this in general?

So, the way Arcanist works is when you arc diff a commit, arcanist edits the commit message to put some metadata in it; if you resubmit that commit then it gets associated with the revision, and the history is kept.

I think it's OK to keep this diff in one diff, it's just a note for future reference.

docs/users_guide/packages.rst
151

Let's say you have -package p-0.2 -package p-0.1. The second package flag undos the first one, so only p-0.1 is exposed.

However, if you have -hide-all-packages -package p-0.2 -package p-0.1, GHC decides to trust you, and brings both versions of p into scope. So that's the behavior difference.

388

"So for example, if you have GHC_PACKAGE_PATH and -package-db, this is effectively equivalent to GHC_PACKAGE_PATH.) Right below here seems reasonable.

393

Perhaps, I misunderstood what you said. I guess what you said applies only when there is an IPID conflict and not applicable for version conflict. So there is no inconsistency.

That's correct.

Why don't we do this in general?

Although it is a sharp edge, it is technically a *feature* that GHC can link against multiple versions of a package. Occasionally it is very useful: perhaps you are testing bytestring and the test suite runner uses bytestring internally (but these are two different versions of the package), or maybe you want to run GHCi, and maybe the package database is a little incoherent but it's OK, you're not using the bad packages together.

In any case, the policy decision here is one GHC leaves in the hands of cabal-install and Stack: it's the job of those programs to make sure the users sees the right package model.

harendra added inline comments.Aug 24 2016, 12:44 AM
docs/users_guide/packages.rst
151

I see, this needs to be illustrated in the manual.

harendra updated this revision to Diff 8467.Aug 24 2016, 6:23 AM
harendra edited edge metadata.
  • Packages doc: address review comments by ezyang

I uploaded another commit. I think I addressed all the comments. Please go through it again and let me know if there is anything which is inaccurate or needs to be reworked.

docs/users_guide/packages.rst
388

After rework this is more of an abstract overview rather than specific details. So I added the example in GHC_PACKAGE_PATH section instead. Let me know if you agree with that.

ezyang accepted this revision.Aug 30 2016, 12:43 AM
ezyang edited edge metadata.

LGTM!

Macro shipitquick:

docs/users_guide/packages.rst
94–98

A hidden package may still be a dependency of an exposed package; hiding a package only hides its modules from direct imports.

This revision is now accepted and ready to land.Aug 30 2016, 12:43 AM
This revision was automatically updated to reflect the committed changes.