-
Notifications
You must be signed in to change notification settings - Fork 701
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
Global Constraints Option #2820
Conversation
/cc @dcoutts |
As a minimum, this needs documentation. |
@23Skidoo sure. I guess this can be added in installing-packages.markdown? Maybe under miscellaneous options? It looks to me we don't have a good place that collects all options settable in |
I think so. It'd be nice to have a section describing the config file, but that can wait of course. |
Ok, documentation added. |
minp <- readPackageEnvironmentFile ConstraintSourceUserConfig mempty path | ||
case (minp, globalConfigLocation) of | ||
(Just parseRes, _) -> processConfigParse path parseRes | ||
(_, Just globalLoc) -> maybe (warn verbosity ("no constraints file found at " ++ path) >> return mempty) (processConfigParse globalLoc) =<< readPackageEnvironmentFile ConstraintSourceUserConfig mempty globalLoc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor ConstraintSourceUserConfig
should probably be updated to take the path to the constraints file. Currently, the location is hard-coded as "cabal.config" in showConstraintSource
:
showConstraintSource ConstraintSourceUserConfig = "cabal.config" |
as per @grayjay 's comment, ConstraintSourceUserConfig now takes a path to improve messages. |
So... if I add a cabal.config file, the global one doesn't apply at all? |
That's the proposal in how this is implemented. The idea on my part is that a "local freeze" should be definitive -- it captures all the things for a build. So if the global file still had any effect, that would disrupt this. Arguably this is odd -- from the standpoint of the command line, you really want it to override the local, since obviously why else would you pass the command. But from the standpoint of cabal/config, you would tend to want the local to override it. (This gets to how we sort of need a more uniform story for options and phases, etc). So I think the current behavior is ok, but would be open to simply removing the command line flag an only recognizing this in cabal/config, since that's really where the option is mainly targeted for. |
I guess what's totally unclear to me is how the Platform is actually going to use this functionality. Like, "X is the user experience we want to give users, and the platform does Y, and so Cabal needs to support Z." The global file "disappearing" when you add a local file... sounds like bad user experience. But I don't know! I don't know how to use this. |
The key behavior is only how it acts in cabal/config files, not on the command line. Here's the scenario: we intend to make the platform "minimal" by not bundling additional packages beyond core. But now, in what sense do packages like Text, etc, which have been included in the platform, "exist" as platform packages? This patch is intended to address that question. Much like how stackage works, the platform can nonetheless exist as a "global constraint set" such that if you are on a particular platform, then it ships with a cabal/config that points to a particular platform's constraints file. So now, you are constrained to a "platform approved Text" even though Text itself did not ship as a preinstalled binary. One could imagine more complex tools, such as additional build-dbs in the middle, etc. This could provide additional caching of shared artifacts between dependencies, etc. But such tools involve larger refactors, have the potential to step on other work in progress, etc. So this is a very minimal patch to at least accomplish the core goal. Now, as to why it is overriden by a local file. The point is a local "cabal.config" is supposed to be a freeze file that strictly pins versions. So it should take precedence over everything else. There are three scenarios where such a file arises -- one, you explicitly create one based on a call to the "cabal freeze" command -- that command will freeze based on the install plan taking this flag into account. So in scenario one your freeze is compatible with this constraint set to begin with. Two, you explicitly create one by hand, or by modifying an auto-generated one. In this case, any choices you make to override are clearly intentional. Three, you download one with a chunk of code. In that case, the explicit intent of the author of that code is to pin to the versions in the freeze file, and letting the global default constraints interact would be contrary to that. So the idea is that the global default constraints are a "guidepost to platform compatibility" or the like, and also gives some form of meaning to packages included in the platform (as a way of both 'blessing' them and also indicating that they are validated to all work together). |
(Bear in mind, to be clear, that this is not overridden by a local "Foo.cabal" file -- it is overridden by an explicit "freeze" or constraints file -- i.e. a "cabal.config" file). |
OK LGTM. |
ok so I guess I have perms to merge this PR. Any objections? |
So I'd like to double check how this relates/compares to the proposed package collections feature. Obviously with package collections there's also the option to host on a hackage server, but to keep things simple, lets compare with a package collection that's stored locally in a file. So lets suppose we have a "haskell platform" collection, and the HP ships cabal-install with a default config file that specifies to use that collection. So how would the behaviour be the same or different to what this patch does? The proposed behaviour for the collection is that (like most other config options) it's overridden in a local cabal.config, but only if the cabal.config actually specifies a collection. A freeze command would similarly start from the current config, which would use the collection. I'd like to understand this so that when we do add collections support (possibly in time for the ghc-8.0 release) we know if we can replace the mechanism in this patch, or if there's still some other differences we'd need to account for. One thing I'm not entirely clear on: the code for |
I don't fully understand the collections feature to be honest, since there was just the original proposal and whatever is in the branch, and I haven't sorted that through. That said, I guess this is basically more general -- since as you point out it actually provides a global "extra config" as opposed to simply more constraints. The command line probably should be bikeshedded to match -- just changing it to So you write:
The difference here is that a local cabal.config would subsume this config entirely -- I.e. this only gets used if there is not a user-config in place. So a cabal.freeze that creates a user-config would respect this in the solving portion. But from then on out the cabal.config thus created would then subsume this. To me, the advantage here is that this is a modest patch that just makes the existing user-config mechanism slightly more general, and is justifiable as that alone. But then, once it is in place, it also seems to perhaps obviate the need for a whole separate setup for collections? I.e. what collections bring that this doesn't is just the "remote location" case as I understand it -- and that can be accomplished just by allowing the argument to this to be a url as well as a local file? |
Ok, so if we assume for the moment that we're using this just for constraints and not other options, then what is the behaviour in each case: Initial state with no local cabal.config, user runs:
With this patch (correct me if I'm wrong), the first uses the global default platform constraints. The second uses those same global constraints as the input to the solver when we run freeze. Finally, after freeze now that we've written a local cabal.config we get the constraints from the cabal.config. If we use a package collection and specify the default collection in the user wide ~/.cabal/config then I think the behaviour is the same. In the first and second cases we get the collection from the global config. In the third, assuming we adjust the implementation of freeze when we add the collections feature, then it would create the local cabal.config telling it to use a particular collection (initially with no local variation, but a user could edit it to add local variation). So I'm not sure I see a difference.
But if a local cabal.config specifies a package collection overriding a user-wide setting, then isn't that the same as a local cabal.config completely overriding a user-wide config file containing only constraints? Is this just a difference of mechanism that gives the same result in the end or is there some behavioural difference I'm missing. I'm not asking to say we shouldn't do this patch now. I'm asking because I want to know if we replace this patch later with a somewhat more fully fledged package collection feature (even if we miss out the remote collections & hosting initially), are we're covering the same required behaviour. Incidentally, a reason we would want to support the notion of a collection directly, rather than just as a config file with a collection of constraints is that we can give better messages and we have more flexibility on tweaking behaviour. For example we could easily use a collection as a preference rather than as a hard constraint (and have that be easily user configurable). |
You're right the functionality is precisely the same in those three cases. The case they diverge is say the user instead of running "cabal freeze" just manually created a "cabal.config" file with no constraints. Under this patch, a subsequent "cabal install" would not use any constraints. Under the collections functionality, since the collection wasn't overridden, then the global collection would still be respected. That's the only difference I can see in terms of a possible workflow, and it doesn't seem a very common case. Also I don't think either behavior is necessarily better -- they just follow from the two approaches implementing similar functionality via different angles. |
Ok, updated the command name as per this discussion. After the build passes I'm going to merge unless there are further considerations? |
I'm OK with merging this. I guess this feature will be only used by HP, which can later migrate to proper collections once that is implemented.
This is actually a supported use case: e.g. one may want to set some |
This lets you either pass
constraints-file
on the command line or inthe ~/.cabal/config file.
The constraints-file serves as a "default" cabal.config file for
freezes should one not be provided by a particular package.
Thus, a "minimal" platform can ship with a file that includes the
platform constraints. Later, a user can simply download a new file for
a new constraints set -- say lts -- and swap their config to point to
that instead.
This makes both the platform in its new ideally-more-minimal guise and
stackage play nicer together and with cabal, in a way that required
minimal changes to cabal proper.
Ideally this or a relative can land soon to help enable the minimal
platform plans (the work grew out of the discussion on:
haskell/haskell-platform#206)
In PackageEnvironment.hs, the tryLoadSandboxPackageEnvironmentFile
doesn't check if this flag is set and handle that case. So this is
when we are already in a sandbox and the question is if we should
also respect this flag, just as a sandbox also respects a
cabal.config file directly in the directory. My gut says "don't
respect it in sandboxes" which is what I now have implemented, but this is open for discussion.
Freeze.hs does respect this flag.