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.

Sign up for GitHub

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

Jump to bottom

Discard "internal heuristics", add builtin solver using ocaml-mccs #3011

Merged
merged 25 commits into from
Aug 17, 2017
Merged

Discard "internal heuristics", add builtin solver using ocaml-mccs #3011

merged 25 commits into from
Aug 17, 2017

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Jul 31, 2017

Note: not for merge right away, this needs the new build-system to be finalised first

@AltGr AltGr force-pushed the ocaml-mccs branch 3 times, most recently from 53c50eb to b871382 Compare August 3, 2017 08:40
@AltGr
Copy link
Member Author

AltGr commented Aug 8, 2017

Now based on the merged jbuilder patches, and cleaned up (supporting build without mccs): should be ready.

@dra27
Copy link
Member

dra27 commented Aug 8, 2017

Can I have until Friday afternoon to try this on Windows and review further?

One immediate thought: I think the dynamic selection of static-linking.sexp is wrong (because it's not future-proof). I would either have it that configure copies static-linking.sexp.true if static linking is wanted and the jbuild rule is just (with-stdout-to ${@} (echo "()")) with no dependencies (so you get dynamic linking if you build without running configure) or, and this is probably better, define an additional package opam-static in src/client/jbuild with a different library and push the static/dynamic option back to ocaml-mccs (so opam-mccs also defines a findlib package ocaml-mccs.static).

@AltGr
Copy link
Member Author

AltGr commented Aug 8, 2017

Going through the env is clearly a temporary hack, until there is a better way to select the version we want. Two different targets/aliases would nicely do the trick, but I am not sure how they would relate to the opam package (which shouldn't include both... ?)

@AltGr AltGr force-pushed the ocaml-mccs branch 5 times, most recently from 987d06f to c04c318 Compare August 14, 2017 13:30
@AltGr
jbuilder: use ocaml-mccs
68a2fd1
@AltGr
Refactor solver configuration, unifying setup of built-in and externa…
1bdd49d 
…l solvers

Also, disables the "internal heuristics" solver (but this patch doesn't
remove it yet)

This also makes the 'best-effort' parameter configurable.
@AltGr
Add the ocaml-mccs dependency to the libs
829502c
@AltGr
Separate builtin solver to its own file
0ccf5a9 
so that the file can easily be swapped with a dummy one with `is_present
= lazy false` and no implementation, when compiling without Mccs.
@AltGr
lib-ext: add ocaml-mccs 1.1+1c
5c5c90c
@AltGr
Cleanup, fix and simplify configure/make; make mccs optional
c825bfb
@AltGr
opamCudfSolver code simplification
fa308d2
@AltGr
Add rules for static linking
13303c8 
'make opam-static'
@AltGr
Tweak aspcud's default criteria
e6fd64b 
- choose newer version more agressively (put uptodate criteria before
  minimise changes criteria). This prevents many cases of getting a very
  old version of a lib because newer ones have new requirements (e.g.
  `base-bytes`)
- remove duplication that was there to guarantee compat with much older versions:
  `-notuptodate(foo),-sum(foo,version-lag)` is now just
  `-sum(foo,version-lag)`. While slightly different, it isn't worse, and
  is simpler.
  It is also closer to the criteria we use for mccs.
@AltGr
Remove the older OpamHeuristics module and support code
a6021e4 
Closes #2989
@AltGr
Travis: add libglpk dependency
d606379
@AltGr
configure: check for glpk, fail if '--without-mccs' can't be respected
472562a
@AltGr
Appveyor: add libglpk from cygwin
0ee6554
@AltGr
Jbuilder bootstrap Makefile fixes
0b53bb2
@AltGr
Update mccs dependency (no gcc-specific static linking by default)
045ad83
@AltGr
Travis: some fixes
43c00bb 
(don't pin to dev in the bootstrap phase, that should remain as stable
as possible!)
@AltGr
make lib-ext: clean up output, small fixes
0ddaf28
@AltGr
Test with newer cudf
d640b95
@AltGr
Copy link
Member Author

AltGr commented Aug 15, 2017

Mostly OK now. Remaining issues:

  • OCAML_VERSION=4.03.0 OPAM_TEST=1 EXTERNAL_SOLVER= glpk wasn't found, and opam was compiled without internal sover. Therefore, forcing the internal solver fails.
  • OCAML_VERSION=4.03.0 OPAM_TEST=1 EXTERNAL_SOLVER=aspcud the check for aspcud version seems broken (on older aspcud)
  • OCAML_VERSION=4.05.0 OPAM_TEST= tests from the opam repo seem to fail, need to investigate, but this test is new
  • osx OCAML_VERSION=4.03.0 OPAM_TEST=1 EXTERNAL_SOLVER= seems that re is still missing ?
  • osx OCAML_VERSION=4.03.0 OPAM_TEST=1 "no external solver found" ?

@AltGr AltGr force-pushed the ocaml-mccs branch 2 times, most recently from 24764f6 to a2d66f0 Compare August 15, 2017 13:34
@AltGr
Copy link
Member Author

AltGr commented Aug 15, 2017

Ah, the bootstrap opam (1.2.2) doesn't correctly extract the opam file from the ocaml-mccs archive.
This means we're stalled by ocaml/opam-repository#10075

@AltGr AltGr force-pushed the ocaml-mccs branch 2 times, most recently from 34b1409 to e429939 Compare August 16, 2017 13:15
@AltGr
Copy link
Member Author

AltGr commented Aug 16, 2017

Finally ! All tests pass :)

@AltGr AltGr merged commit 51a58f4 into master Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@AltGr @dra27