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

build/pkgs/{gmp,mpfr}/spkg-configure.m4: Check pkg-config first #31348

Open
mkoeppe opened this issue Feb 6, 2021 · 43 comments
Open

build/pkgs/{gmp,mpfr}/spkg-configure.m4: Check pkg-config first #31348

mkoeppe opened this issue Feb 6, 2021 · 43 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 6, 2021

Currently gmp.pc, which may be provided at least on some platforms, is not used at all -- neither by configure nor by sage.env.cython_aliases, sage.misc.cython.

For example, on homebrew, using it will provide the specific library search directory -L/usr/local/Cellar/gmp/6.2.1/lib instead of having to rely on /usr/local/ (which is disabled when compiler/linker are invoked with -isysroot)

Currently, gmp is detected using AC_CHECK_HEADER, and the result is communicated to sage-build-env-config as variables SAGE_GMP_PREFIX and SAGE_GMP_INCLUDE; but it is not available in the runtime environment (sage_conf.py, sage-env-config).

The failures reported in

(After #32549, the relevant file is build/pkgs/gmp/spkg-configure.m4.)

The above applies to mpfr as well. See failure report in https://groups.google.com/g/sage-release/c/8ctwsUGl6LQ/m/Ob38Iyh1AAAJ

Similar situation with readline (#29000): The spkg-configure script already uses pkg-config - but forgets to tell the build system what it found.

Previous tickets: #27212

Depends on #31344
Depends on #32549

CC: @dimpase @jhpalmieri @zlscherr @orlitzky

Component: build: configure

Keywords: mpfr

Branch/Commit: u/gh-zlscherr/add_gmp_mpfr_cython @ 07e00b6

Issue created by migration from https://trac.sagemath.org/ticket/31348

@mkoeppe mkoeppe added this to the sage-9.3 milestone Feb 6, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title build/pkgs/mpir/spkg-configure.m4: Check pkg-config first build/pkgs/{mpir,mpfr}/spkg-configure.m4: Check pkg-config first Feb 6, 2021
@zlscherr
Copy link

zlscherr commented Feb 9, 2021

comment:4

I started to play around with this a bit, and I can use python’s pkgconfig to locate gmp and mpfr. After doing that though, I get complaints about not being able to locagte -lntl, which I don’t think I can find with pkgconfig since it doesn’t have an ntl.pc file.

This may be a bit too hacky, but would it be possible in the build process to copy the various includes and libraries to SAGE_LOCAL/{include, lib} that are picked up by configure? That seems like it would solve a whole host of problems at the expense of if the user updates one of those libraries then the new version would not be picked up.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 9, 2021

comment:5

Replying to @zlscherr:

I started to play around with this a bit, and I can use python’s pkgconfig to locate gmp and mpfr. After doing that though, I get complaints about not being able to locagte -lntl, which I don’t think I can find with pkgconfig since it doesn’t have an ntl.pc file.

For NTL, the configure script already determines SAGE_NTL_PREFIX.
This variable just needs to be used more.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 9, 2021

comment:6

I've opened #31365 for NTL.

@zlscherr
Copy link

zlscherr commented Apr 7, 2021

Branch: u/gh-zlscherr/add_gmp_mpfr_cython

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 7, 2021

Commit: 07e00b6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 7, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

07e00b6Modify stdlibs/incs in misc/cython.py

@zlscherr
Copy link

zlscherr commented Apr 7, 2021

comment:9

I pushed very simple code for trying to deal with this. It's probably better to use regular expressions, but I just wanted a proof of concept. This allows make ptest to work with homebrew without having to source .homebrew-build-env.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@roed314
Copy link
Contributor

roed314 commented Apr 26, 2021

comment:13

I've started getting failures that are fixed by sourcing .homebrew-build-env even when I'm just changing Cython files and recompiling (not changing any spkgs). Is this expected?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 26, 2021

comment:14

Yes, this is part of what this ticket intends to fix.

@roed314
Copy link
Contributor

roed314 commented Apr 26, 2021

comment:15

Great, thanks for the clarification!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 10, 2021

comment:16

Moving to 9.4, as 9.3 has been released.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 May 10, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@mkoeppe

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Sep 23, 2021

comment:19

some distros, like Fedora, have an unpleasant habit of not installing .pc files, while these are available.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2021

comment:20

Yes, the ticket is "Check pkg-config first", not "Check pkg-config only".

@orlitzky
Copy link
Contributor

comment:21

IMO homebrew (and anyone else installing libgmp to a nonstandard directory) should be pestered to install it to the system's default location. I'm pretty sure that the gmp and mpir packages then conflict, but you don't need to install them both at the same time. In fact, mpir can probably be deprecated.

Using pkg-config just changes this potential problem (libraries in wrong locations) to another one (pc files in wrong locations) and adds an extra layer of indirection in the process. We may have to go ahead and fix it anyway, but we shouldn't lose track of the simple and easy way that this was all meant to work.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 25, 2021

comment:27

It's really just the broken design of homebrew's python3. They use -isysroot in the build configuration of python, which goes into the compiler/flags of any extensions built via distutils/setuptools. Then they add back a few homebrew directories via distutils.cfg. It's hopelessly broken because these directories take priority over other user/package-provided include and linker directories. At build time, we work around it by disabling use of the system python's vendored distutils and use our own.
At runtime, we would be best off by using paths explicitly that can be discovered via pkg-config -- because in the Sage library we already do that for all other packages.

@dimpase
Copy link
Member

dimpase commented Sep 25, 2021

comment:28

is this reported to Homebrew as a bug?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 25, 2021

comment:29

Replying to @dimpase:

is this reported to Homebrew as a bug?

I didn't report it when I came across it in #31335. I figured a bug report titled "distutils.cfg considered harmful" would drain a lot of energy...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 25, 2021

Changed dependencies from #31344 to #31344, #32549

@dimpase
Copy link
Member

dimpase commented Sep 26, 2021

comment:31

Replying to @mkoeppe:

Replying to @dimpase:

is this reported to Homebrew as a bug?

I didn't report it when I came across it in #31335. I figured a bug report titled "distutils.cfg considered harmful" would drain a lot of energy...

You don't need to fight a battle, merely report the problem, please.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 27, 2021

comment:32

They're pretty quick with closing as wontfix if one doesn't also provide a patch

@dimpase
Copy link
Member

dimpase commented Sep 27, 2021

comment:33

Replying to @mkoeppe:

They're pretty quick with closing as wontfix if one doesn't also provide a patch

Sprecht du kein Ruby, oder? :-)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 12, 2021

comment:34

Replying to @dimpase:

Replying to @mkoeppe:

Replying to @dimpase:

is this reported to Homebrew as a bug?

I didn't report it when I came across it in #31335. I figured a bug report titled "distutils.cfg considered harmful" would drain a lot of energy...

You don't need to fight a battle, merely report the problem, please.

I sent a first PR to reduce homebrew's distutils.cfg - Homebrew/homebrew-core#91043

@slel
Copy link
Member

slel commented Jan 1, 2022

comment:35

Removed mpir from ticket summary after its removal in #32549, #32727.

@slel
Copy link
Member

slel commented Jan 1, 2022

Changed keywords from none to mpfr

@slel

This comment has been minimized.

@slel slel changed the title build/pkgs/{mpir,mpfr}/spkg-configure.m4: Check pkg-config first build/pkgs/mpfr/spkg-configure.m4: Check pkg-config first Jan 1, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Jan 10, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 8, 2022

comment:38

Replying to @mkoeppe:

I sent a first PR to reduce homebrew's distutils.cfg - Homebrew/homebrew-core#91043

They have merged a broader change now, eliminating all of distutils.cfg (Homebrew/homebrew-core#91043 (comment)), at least for their Python 3.10 package. That's a good improvement, but it changes nothing for the present ticket.

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 7, 2022
@mkoeppe mkoeppe changed the title build/pkgs/mpfr/spkg-configure.m4: Check pkg-config first build/pkgs/{gmp,mpfr}/spkg-configure.m4: Check pkg-config first May 16, 2022
@dimpase
Copy link
Member

dimpase commented May 20, 2022

comment:41

so this needs changes to gmp's and mpfr's spkg-configure.m4 ? Anything else?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 20, 2022

comment:42

sage.env.cython_aliases also needs to be extended

@dimpase
Copy link
Member

dimpase commented May 30, 2022

comment:43

should we remove all the crud needed for gmp and mpfr without .pc files available?

I suppose almost every platform we support nowadays will have them installed.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 30, 2022

comment:44

Replying to @dimpase:

I suppose almost every platform we support nowadays will have them installed.

That would be worth checking.

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
vbraun pushed a commit to vbraun/sage that referenced this issue Dec 21, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
The unreleased 2.2.x series has work on support for Python 3.12

https://github.com/aleaxit/gmpy

The new version makes it necessary to tighten the requirements for MPC
and MPFR.
We do the version check without pkg-config as before; changing it to
pkg-config is sagemath#31348.


<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36775 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36351
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this issue Jan 5, 2024
…6.3, `gmpy2` 2.2.0a1

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
The unreleased 2.2.x series has work on support for Python 3.12

https://github.com/aleaxit/gmpy

The new version makes it necessary to tighten the requirements for MPC
and MPFR.
We do the version check without pkg-config as before; changing it to
pkg-config is sagemath#31348.

(The upgrade of the packages was previously done in sagemath#36775, but has now
been merged here.)

<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36351
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this issue Jan 13, 2024
…6.3, `gmpy2` 2.2.0a1

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
The unreleased 2.2.x series has work on support for Python 3.12

https://github.com/aleaxit/gmpy

The new version makes it necessary to tighten the requirements for MPC
and MPFR.
We do the version check without pkg-config as before; changing it to
pkg-config is sagemath#31348.

(The upgrade of the packages was previously done in sagemath#36775, but has now
been merged here.)

<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36351
Reported by: Matthias Köppe
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants