Skip to content
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

Merged
merged 5 commits into from Nov 13, 2015
Merged

Global Constraints Option #2820

merged 5 commits into from Nov 13, 2015

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Sep 11, 2015

This lets you either pass constraints-file on the command line or in
the ~/.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.

@23Skidoo
Copy link
Member

/cc @dcoutts

@23Skidoo
Copy link
Member

As a minimum, this needs documentation.

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 11, 2015

@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 config at the moment, so that is perhaps future work as well, even though it largely overlaps with cmd-line flags afaik...

@23Skidoo
Copy link
Member

I think so. It'd be nice to have a section describing the config file, but that can wait of course.

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 16, 2015

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
Copy link
Collaborator

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

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 18, 2015

as per @grayjay 's comment, ConstraintSourceUserConfig now takes a path to improve messages.

@ezyang
Copy link
Contributor

ezyang commented Sep 24, 2015

So... if I add a cabal.config file, the global one doesn't apply at all?

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 24, 2015

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.

@ezyang
Copy link
Contributor

ezyang commented Sep 24, 2015

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.

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 24, 2015

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

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 24, 2015

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

@ezyang
Copy link
Contributor

ezyang commented Sep 24, 2015

OK LGTM.

@gbaz
Copy link
Collaborator Author

gbaz commented Nov 2, 2015

ok so I guess I have perms to merge this PR. Any objections?

@dcoutts
Copy link
Contributor

dcoutts commented Nov 3, 2015

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 --constraints-file isn't really a constraints file it's a general cabal config file which is used if there's no ./cabal.config file right? So it's not limited to providing constraints.

@gbaz
Copy link
Collaborator Author

gbaz commented Nov 3, 2015

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 default-user-config .

So you write:

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.

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?

@dcoutts
Copy link
Contributor

dcoutts commented Nov 3, 2015

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.

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:

cabal install     # what solver input constraints are used here?
cabal freeze    # what solver input constraints are used here?
cabal install     # what solver input constraints are used here?

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.

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.

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

@gbaz
Copy link
Collaborator Author

gbaz commented Nov 3, 2015

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.

@gbaz
Copy link
Collaborator Author

gbaz commented Nov 12, 2015

Ok, updated the command name as per this discussion. After the build passes I'm going to merge unless there are further considerations?

@23Skidoo
Copy link
Member

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.

The case they diverge is say the user instead of running "cabal freeze" just manually created a "cabal.config" file with no constraints.

This is actually a supported use case: e.g. one may want to set some ghc-options there for development and share that file with the team via the common repo.

gbaz added a commit that referenced this pull request Nov 13, 2015
@gbaz gbaz merged commit 1578c4b into haskell:master Nov 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants