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

Make the solver aware of pkg-config constraints #3023

Merged
merged 1 commit into from Mar 5, 2016

Conversation

Projects
None yet
6 participants
@garetxe
Copy link
Contributor

garetxe commented Jan 3, 2016

This pull request fixes #3016 (it does not entirely fix #335, but it goes in that direction). As explained in more detail in #3016, having the solver aware of available pkg-config dependencies is important for bindings for recent versions of external APIs.

The implementation is fairly similar to that in #2873, the only noteworthy point is that on startup we scan the list of all installed pkg-config packages in order to obtain their versions. Obtaining the list of installed packages is straightforward (pkg-config --list-all), based on this list we then call pkg-config --modversion pkg1 pkg2....

If there are a lot of packages the commandline can get somewhat long (4-10 KB), but this should not be a problem in modern operating systems. An alternative would be to call pkg-config --modversion repeatedly for each package, but from quick testing this makes things much slower (0.02s vs 0.5s in my system).

@garetxe garetxe force-pushed the garetxe:master branch from 29fef36 to 4f645a2 Jan 3, 2016

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Jan 3, 2016

Have you tested on Windows? There's a fairly small maximum command line length which, if your command lines are really 10KB large, you are almost definitely going to hit.

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Jan 3, 2016

No, unfortunately I don't have access to a Windows machine where I could test this.

Nevertheless from reading the docs at
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425%28v=vs.85%29.aspx
I was under the impression that the limit for CreateProcess in Windows is 32K, which should be fine.

I believe the limit you mention is for CMD.EXE, but from my reading of the code getDbProgramOutput does not use that, it directly uses CreateProcess under the hood. But I could well be wrong, one should check.

But using such a long command line is certainly somewhat brittle... I am not sure about the preferred design. I could easily chunk the calls to pkg-config --modversion into groups of 100, say. This should be well below the limit, I think. (For reference: in my system, Fedora 22, pkg-config --list-all lists 408 packages, for a command line to pkg-config --modversion of 5194 bytes)

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Jan 3, 2016

You are right, I misstated the limit. I think this should be fine!. LGTM.

(It would be nice if we could get some tests, but pkg-config seems a bit too difficult to actually mock in a useful way.)

On January 3, 2016 10:26:35 AM EST, "iñaki García Etxebarria" notifications@github.com wrote:

No, unfortunately I don't have access to a Windows machine where I
could test this.

Nevertheless from reading the docs at
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425%28v=vs.85%29.aspx
I was under the impression that the limit for CreateProcess in
Windows is 32K, which should be fine.

I believe the limit you mention is for CMD.EXE, but from my reading
of the code getDbProgramOutput does not use that, it directly uses
CreateProcess under the hood. But I could well be wrong, one should
check.

But using such a long command line is certainly somewhat brittle... I
am not sure about the preferred design. I could easily chunk the calls
to pkg-config --modversion into groups of 100, say. This should be
well below the limit, I think. (For reference: in my system, Fedora 22,
pkg-config --list-all lists 408 packages, for a command line to
pkg-config --modversion of 5194 bytes)


Reply to this email directly or view it on GitHub:
#3023 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Jan 3, 2016

CC @edsko and @kosmikus

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Jan 4, 2016

You are right, I misstated the limit. I think this should be fine!. LGTM.

Great, thanks for taking a look!

(It would be nice if we could get some tests, but pkg-config seems a bit too difficult to actually mock in a useful way.)

Indeed it would be nice. I took a look to the testing code in #2873, but I need to spend a bit more time with it to understand how that code works. In any case if you think that the basic approach to modification of the solver in the patch is OK I would be happy to write some tests, trivial as they may end up being (better than nothing at all, I suppose).

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Jan 4, 2016

The fact that in some (extremely unlikely, as discussed above, but possible in principle) cases the initial population of the pkg-config database may potentially fail was bothering me a little, so d46e363 addresses this by switching off the pkg-config check in the solver when this happens.

So in the normal case we have the check, but we revert to the previous situation if readPkgConfigDb fails.

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Jan 4, 2016

Sweet. Maybe make the error message be a little clearer about what the failure means? (Cabal will continue without solving for pkg-config constraints.)

@garetxe garetxe force-pushed the garetxe:master branch from e9f8e48 to 8cc3241 Jan 4, 2016

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Jan 4, 2016

Sweet. Maybe make the error message be a little clearer about what the failure means? (Cabal will continue without solving for pkg-config constraints.)

Sure, done!

I also added some unit tests, and squashed all changes into a single commit, hopefully this makes review a little bit easier.

@garetxe garetxe force-pushed the garetxe:master branch 5 times, most recently from 8050fe7 to 3b465c5 Jan 9, 2016

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Jan 9, 2016

There were some merge conflicts against HEAD, so I have rebased the patch.

Is there anything else you think I should improve in the patch? I would be happy to do so.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jan 11, 2016

@garetxe

I think I'll postpone merging this PR until the 1.24 branch is created. The 1.24 release is imminent and I don't want to introduce such large changes. Hope it's okay with you.

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Jan 11, 2016

@23Skidoo Sure, that is very reasonable! As long the functionality gets in eventually in some form I am happy :)

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Jan 14, 2016

I've not reviewed the code, but in principle this is defiantly a good thing. I'd like to get the solver to take into account more of these things. The big one will be build-tools.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Jan 15, 2016

@23Skidoo Unless the branch really is imminent could you perhaps add a label to the issues which you want to postpone until post-branch? (Just so that I and others don't accidentally merge stuff if we miss one of your comments.)

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jan 15, 2016

@BardurArantsson Sure, will do.

@23Skidoo 23Skidoo added the post-1.24 label Jan 15, 2016

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Jan 16, 2016

@23Skidoo Great, thanks!

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 5, 2016

I've briefly looked at the part of the PR that actually makes changes to the modular solver, and these look generally ok to me.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 5, 2016

I'm going to just merge this based on the LGTMs, my own (possibly naïve) LGTM and the fact that 1.24 has been branched.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 5, 2016

(Uh, hang on... I can't merge it -- there are conflicts. My bad.)

@garetxe garetxe force-pushed the garetxe:master branch 2 times, most recently from 34fa6e1 to 3af1c19 Mar 5, 2016

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Mar 5, 2016

I just rebased, it should merge cleanly now. Thanks for the reviews!

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Mar 5, 2016

Mmm, looking to the automatic checks that just ran one of them (appveyor) is failing, apparently due to the fact that it is in running in Windows, so it cannot find pkg-config, and an unexpected warning is printed (cabal itself should work fine even if pkg-config is missing, in this case we simply revert to the behavior previous to the patch). But if I add the warning to the expected output of the test the tests will fail in linux, or in general when pkg-config is available...

Is there a way of having pkg-config available for the Windows testing machines? Or a way of modifying the test so that the warning is ignored?

@garetxe garetxe force-pushed the garetxe:master branch from 3af1c19 to aa2ec04 Mar 5, 2016

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 5, 2016

@garetxe Windows binaries for pkg-config and its dependencies are available here; installation instructions are available [here(http://stackoverflow.com/a/22363820/242169). We can change the appveyor.yml script to download and install them during the setup phase.

-- too long command line).
ioErrorHandler :: IOException -> IO PkgConfigDb
ioErrorHandler e = do
info verbosity ("Failed to query pkg-config, Cabal will continue without solving for pkg-config constraints: " ++ show e)

This comment has been minimized.

@23Skidoo

23Skidoo Mar 5, 2016

Member

Please respect the 80-col line length limit.

This comment has been minimized.

@garetxe

garetxe Mar 5, 2016

Contributor

Thanks, fixed!

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 5, 2016

LGTM as well.

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Mar 5, 2016

@23Skidoo Thanks!

In any case I just updated the patch in the pull request to use info instead of warn when telling the user that pkg-config was not found, so it only appears in verbose mode. This should make the test pass.

This was probably the right thing to do in the first place anyway. It could be a bit annoying to see the message every time if one was interested in using cabal without having pkg-config around, which does make sense.

Make the solver aware of pkg-config constraints
When solving, we now discard plans that would involve packages with a
pkgconfig-depends constraint which is not satisfiable with the current
set of installed packages (as listed by pkg-config --list-all).

This fixes #3016.

It is possible (in principle, although it should be basically impossible
in practice) that "pkg-config --modversion pkg1 pkg2... pkgN" fails to
execute for various reasons, in particular because N is too large, so
the command line becomes too long for the operating system limits.

If this happens, revert to the previous behavior of accepting any
install plan, regardless of any pkgconfig-depends constraints.

@garetxe garetxe force-pushed the garetxe:master branch from aa2ec04 to c72aa8d Mar 5, 2016

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 5, 2016

Yes, I agree that using info is the right approach.

23Skidoo added a commit that referenced this pull request Mar 5, 2016

Merge pull request #3023 from garetxe/master
Make the solver aware of pkg-config constraints

@23Skidoo 23Skidoo merged commit 6c45ec9 into haskell:master Mar 5, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 5, 2016

Merged, thanks!

@23Skidoo 23Skidoo removed the post-1.24 label Mar 5, 2016

@23Skidoo 23Skidoo added this to the cabal-install 1.26 milestone Mar 5, 2016

@garetxe

This comment has been minimized.

Copy link
Contributor

garetxe commented Mar 5, 2016

Great, thanks a lot!

@ezyang ezyang modified the milestones: cabal-install 2.0, 2.0 (planned for next feature release) Sep 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment