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

conf-gmp: use pkg-config where available #23330

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

mseri
Copy link
Member

@mseri mseri commented Feb 16, 2023

See #18129 and #22954

I need to test the dependencies in opam list --depends-on conf-gmp before un-drafting

See ocaml#18129 and ocaml#22954

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@mseri
Copy link
Member Author

mseri commented Feb 16, 2023

Checked packages:

  • bap-std
  • class_group_vdf
  • comby
  • conf-gmp-powm-sec
  • conf-mpfr
  • gappa
  • gmp-ecm
  • goblint
  • guile
  • libsail
  • lutin
  • mlgmp
  • mlgmpidl
  • numerix
  • polka
  • sail
  • secp256k1-internal
  • yices2
  • z3
  • zarith

Upper bound needed for

mlgmp, numerix

although they were already broken in systems with non-standard include paths

@artempyanykh
Copy link

Hi @mseri ! Just wanted to double-check if I could help somehow here. For the context, I'm interested in getting zarith fixes in as otherwise my team can't simply build our stuff (infer) on M1 MacBooks.

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@nberth
Copy link
Contributor

nberth commented Apr 4, 2023

Hi @mseri
I've had similar issues locating weird installation paths for system libraries with mlgmpidl, so I've tried in #23613 an approach where packages like conf-gmp and conf-mpfr define some opam variables that other packages like zarith or mlgmpidl can use to locate the libraries (see https://github.com/ocaml/opam-repository/pull/23613/files#diff-2aa76d47e681a91666af166e62200ce54a0650971fbb9133bf778cf0cf1aeb27R12 for an example usage). For now this attempt makes use of a (slightly hackish) support package ez-conf-lib and does not involve pkg-config, as I've seen some systems where the directories it returns seemed incorrect; I think that feature could still be added with little effort.

@artempyanykh
Copy link

Hi @mseri! I see there's an alternative PR for GMP #23613 . Even if it gets merged, we'll still need a patch to zarith to use pkg-config for flag lookup. Is there something I can help with to help push this PR forward?

@mseri
Copy link
Member Author

mseri commented Jun 8, 2023

I need to check all the packages in the comment above, ensure that they link properly and that their reverse dependencies all still work. Once they all have a checkmark, we can undraft this

@mseri
Copy link
Member Author

mseri commented Jun 8, 2023

There is a PR in zarith to use pkg-config for gmp, which will also align it to this PR

@jvillard
Copy link
Contributor

Any chance this could be revived? gmp and zarith cannot be installed on osx with custom Homebrew paths at the moment.

@nberth
Copy link
Contributor

nberth commented Mar 20, 2024

Any chance this could be revived? gmp and zarith cannot be installed on osx with custom Homebrew paths at the moment.

There's been progress on the mlgmpidl side recently. It now uses a distinct "conf" package, named conf-gmp-paths. Customization via user input is not complete yet, but if that does not install properly you may be able to try the hack I suggested here.
I don't know if there is any plan to adapt zarith accordingly yet.

@mseri
Copy link
Member Author

mseri commented Mar 20, 2024

I will try to take some time to continue, but this is completely manual work and takes a lot of time for each package. I am not sure when I can unblock it.

@mseri
Copy link
Member Author

mseri commented Mar 20, 2024

the change to make zarith use pkg-config has been merged in the meantime (ocaml/Zarith#132) and published in zarith 1.13 (release-1.13)

@mseri
Copy link
Member Author

mseri commented Mar 20, 2024

I think I can refresh this PR with the suggestion from @avsm in #24133

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