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

Solver: Check whether components are buildable in the current environment. #5337

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

grayjay
Copy link
Collaborator

@grayjay grayjay commented May 21, 2018

This commit handles the most common case of issue #5325 by checking that each
component that is required as a dependency is buildable in the current
environment, where environment refers to the compiler, os, arch, and global flag
constraints. The solver records whether each component is buildable in the
package's PInfo during index conversion. Then it checks that each required
component is buildable in the validation phase, similar to the check for missing
components.

The buildable check can give false-positives, because it only considers flags
that are set by unqualified flag constraints, and it doesn't check whether the
intra-package dependencies of a component are buildable. The check is also
incomplete because it is performed before any automatic flags are assigned. It
is possible for the solver to later choose a value for a flag that makes the
package unbuildable.

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

/cc @hvr @phadej

…ment.

This commit handles the most common case of issue haskell#5325 by checking that each
component that is required as a dependency is buildable in the current
environment, where environment refers to the compiler, os, arch, and global flag
constraints.  The solver records whether each component is buildable in the
package's PInfo during index conversion.  Then it checks that each required
component is buildable in the validation phase, similar to the check for missing
components.

The buildable check can give false-positives, because it only considers flags
that are set by unqualified flag constraints, and it doesn't check whether the
intra-package dependencies of a component are buildable.  The check is also
incomplete because it is performed before any automatic flags are assigned.  It
is possible for the solver to later choose a value for a flag that makes the
package unbuildable.
@23Skidoo
Copy link
Member

/cc @kosmikus

@phadej
Copy link
Collaborator

phadej commented May 28, 2018

I used this patch in servant's CI, and it fixes the issue there. So 👍 from me, solves my problem :)

@phadej
Copy link
Collaborator

phadej commented Jun 3, 2018

I have dogfooded this for over a week, so I guess @hvr.

Herbert: what's your experiences, let's merge this!

@hvr
Copy link
Member

hvr commented Jun 7, 2018

I've dogfooded this and it handles the urgent http-client-issue. So I vote for merging this one!

However, in the meantime I found yet another corner case (which this PR doesn't address):

cabal-version: 2.2
name: z
version: 0

library
  build-depends: yesod == 0.0.0.2

which results in

Resolving dependencies...
Error:
    Dependency on unbuildable library from yesod
    In the stanza 'library'
    In the inplace package 'z-0'

is this expected?

@hvr hvr merged commit b747032 into haskell:master Jun 8, 2018
@hvr
Copy link
Member

hvr commented Jun 8, 2018

I've talked to @23Skidoo and this clearly represents an improvement for a rather urgent issue. So I'm merging this, and cherry-picking to the 2.2 branch to extend exposition to a larger user-base (via Travis CI)

@grayjay ...if you figure out a way to address the less urgent yesod problem this can be fixed lateron...

PS: cherry-picked to 2.2 branch, see fe2110d & 4bdf0aa

@hvr hvr added this to the 2.2 milestone Jun 8, 2018
@grayjay
Copy link
Collaborator Author

grayjay commented Jun 9, 2018

Thanks for trying out the PR, and thanks @hvr for cherry-picking the changes onto 2.2!

The example probably failed because yesod-0.0.0.2 has an automatic nolib flag controlling its library, and the solver flipped it to true to avoid an unsatisfiable dependency. This PR doesn't handle that case, because it ignores flags, unless they are set by flag constraints. I'm not sure if there is a good way to fix that specific case without implementing the full fix for #5325. The solver would need to track which components are buildable while choosing flags and track the reasons that components become unbuildable, instead of determining which components are buildable at the start.

@hvr
Copy link
Member

hvr commented Jun 9, 2018

@grayjay hrm... so yesod-0.0.0.2 pkg spec appears ill-specified anyway, as toggling the flag results in the lib-component's build-depends being disabled as well, and since this is a cabal-version:1.6 description, it means that the exe component becomes effectively unusable as well as it e.g. loses its implicitly inherited build-depends on base as well?

@grayjay
Copy link
Collaborator Author

grayjay commented Jun 11, 2018

The build-depends field isn't under the conditional, so it might work with older versions of cabal that required dependencies for unbuildable components.

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.

4 participants