Skip to content

More nix-local-build merging: solver and setup wrapper features #3094

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

Merged
merged 7 commits into from
Feb 1, 2016

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Jan 29, 2016

Mostly two things:

  • solver: allow specifying default setup dependencies for packages that don't specify them explicitly
  • setup wrapper: a couple extra features we will need

See individual patch descriptions for details.

This should have been part of the main patch for this feature.

This part of the feature takes stanza choice nodes that have been marked
as being soft preferences and makes them "weak", in the same sense (and
via the same mechanism) that we have weak flag choices. With this
change, weak flags choices remain as the weakest, with stanza prefs are
second weakest.

The intention is that we don't try too hard to enable optional stanzas,
ie we don't cause too much additional backtracking.
Newer packages can have explicit dependencies for their Setup.hs
scripts. This feature allows us to supply setup dependencies for
packages that do not specify explicit dependencies. This makes it
possible to implement policies like having older custom Setup.hs scripts
depend on an older Cabal lib (e.g. adding a default dep on Cabal < 1.23)

The nix-local-build branch implements this policy and it goes further
and tracks all setup deps so that no "ambient" packages are ever used.
The useDependenciesExclusive in the SetupWrapper controls two bits of
behaviour: if the deps given for building the Setup.hs are the only
ones allowed, and if we should make version cpp macros available when
building the Setup.hs. Currently we either want both behaviours or
neither behaviour. We want them in the case that a package declares a
custom setup with explicit dependencies.

It makes sense however to have one behaviour without the other. In
particular it makes sense if we want to implement a policy where we
supply default dependencies for older packages that have custom setup
scripts but don't specify any deps. In this case we want to make the
default deps exclusive, but we don't want to use the version macros,
because those ought to be reserved for packages that are doing the right
thing and opting-in to the new world of explicitly specified setup deps.

So this patch splits the behaviour into two controls, but still uses
both together so there's no change in behaviour yet.
A new option in SetupScriptOptions to specify exactly what version of
the Cabal spec we believe we're using when we're talking to the
Setup.hs.

Currently the version of cabal to use is guessed by the SetupWrapper
code based on the useCabalVersion version range and what versions are
installed in the ambient package environment, and cached state of
whatever an existing compiled Setup used.

When an explicit useCabalSpecVersion is given, all of these heuristics
are short cut and we use exactly the version we're told to use. In this
case it's the responsibility of the caller to useDependencies with a
suitable Cabal lib version, or otherwise know that we'll be using the
self or internal setup methods.

This approach goes along with the idea of deciding up front (and in a
deterministic way) what setup deps to use (using defaults if needed),
rather than deciding based on what's currently in the user environment.

In the current code paths, useCabalSpecVersion is Nothing, so no change
in behaviour yet.
@23Skidoo
Copy link
Member

Looks good modulo comments on the useCabalSpecVersion patch.

@dcoutts
Copy link
Contributor Author

dcoutts commented Jan 29, 2016

Ok, I think it is a bit different. I wrote that bit some time ago so I'll have a think and try and work out if/why it's different. I suspect there's a behavioural difference in there, but there's also a difference of approach. I think we want to move to this "I'll tell you exactly how it'll be" style rather than the "you work it out based on the current environment" approach. A further change we'll be able to make eventually will be only using useCabalSpecVersion and we'll be able to remove useCabalVersion etc.

@23Skidoo
Copy link
Member

Ok, I think it is a bit different. I wrote that bit some time ago so I'll have a think and try and work out if/why it's different.

OK. To me, useCabalVersion looks like the same thing, but just a bit more general, since it allows to specify both an exact version and a range of versions. If we want to remove the latter feature, maybe we can just change the type of useCabalVersion to Maybe Version?

@dcoutts
Copy link
Contributor Author

dcoutts commented Jan 31, 2016

Ok, so there's a few differences.

  • Firstly the existing logic is a good deal more complicated (so it's a bit tricky to work out that it'll arrive at the same conclusion, and more prone to possible unintended changes over time).
  • The existing code path will load the package index in this situation to confirm that a suitable version of the Cabal lib is installed. This is obviously unnecessary and slow in this case. The SetupScriptOptions do let us pass in a package index to optimise this, however in the nix-local-branch we don't have that available at the time (because we don't need it, we've already decided what versions we're going to use).
  • Minor point, but the existing code path assumes all Custom Setup scripts depend on the Cabal library. Technically all that's required is that they follow the Setup.hs CLI, so with a setup-depends that does not include Cabal the existing code path will fail (the nix-local-build will in this case go by the cabal-version specified in the .cabal file).

So my view as to why this approach is useful to add, is that it cuts out a bunch of code that does complicated expensive work to arrive at a conclusion that we already knew. I think this is a good approach to move to. It's what we try to do in the nix-local-branch: decide as much as possible up front (especially everything about dependencies) and then don't do anything clever later on.

@dcoutts
Copy link
Contributor Author

dcoutts commented Jan 31, 2016

If we want to remove the latter feature, maybe we can just change the type of useCabalVersion to Maybe Version?

Yes, if we can switch over the existing code paths to decide the version up front then that'd be a lot better. It may not be practical to do that however until the "new-build" code path becomes the default and we're able to remove the old code paths.

Or another alternative is two entry points to the SetupWrapper, one that works out lots of things for you (e.g. you just give it the Compiler etc and it fills out the other details) and one that expects you to have decided everything.

Currently we have a mix of both, with lots of the SetupScriptOptions being optional. The places where cabal-install directly delegates to Setup.hs are the places where we currently rely on the SetupWrapper to do slightly clever things.

@23Skidoo
Copy link
Member

Ok, so there's a few differences. [...]

OK, this is convincing. Can you just add a link to this comment to the Haddock comment for useCabalSpecVersion?

@dcoutts
Copy link
Contributor Author

dcoutts commented Jan 31, 2016

@23Skidoo sorry, added in two bits as I didn't see your comment in time. I can squash them if you prefer.

@23Skidoo
Copy link
Member

Looks good now, let's merge.

dcoutts added a commit that referenced this pull request Feb 1, 2016
More nix-local-build merging: solver and setup wrapper features
@dcoutts dcoutts merged commit 6694fe1 into haskell:master Feb 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants