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

py-scipy: update to 1.11.4; fix build on old systems #21875

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

barracuda156
Copy link
Contributor

Fixes: https://trac.macports.org/ticket/68014
Closes: https://trac.macports.org/ticket/68379
Closes: https://trac.macports.org/ticket/68380

Description

Update, fix old systems.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 14.2.1
Xcode 15.1

macOS 10A190
Xcode 3.2

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@michaelld for port py-scipy.

@barracuda156
Copy link
Contributor Author

barracuda156 commented Dec 20, 2023

Apparently python38 subport fails. Let me see what can be done. In the worst case we may peg it to an earlier version.

UPD. Well, yeah:

--->  Building py38-scipy
Executing:  cd "/opt/local/var/macports/build/_opt_svacchanda_SonomaPorts_python_py-scipy/py38-scipy/work/scipy-1.11.4" && /opt/local/Library/Frameworks/Python.framework/Versions/3.8/bin/python3.8 setup.py --no-user-cfg config_fc --fcompiler gnu95 --f77exec /opt/local/bin/gfortran-mp-13 --f77flags='-m64 -Os -fno-second-underscore' --f90exec /opt/local/bin/gfortran-mp-13 --f90flags='-m64 -Os -fno-second-underscore' config --cc /usr/bin/clang --include-dirs /opt/local/include --library-dirs /opt/local/lib build -j8 
Traceback (most recent call last):
  File "setup.py", line 46, in <module>
    raise RuntimeError("Python version >= 3.9 required.")

Support for Python 3.8 dropped here: scipy/scipy@890ffe9
I guess no point to try restoring it. I will peg py38-scipy to 1.10.1.

@barracuda156 barracuda156 marked this pull request as draft December 20, 2023 23:30
@barracuda156
Copy link
Contributor Author

I will hopefully fix this now, gimme few min.

@reneeotten
Copy link
Contributor

@barracuda156 I was working on this update locally and we should be switching to the meson-python backend.

As I said before the issue is that the variants with other BLAS/LAPACK versions like MKL are mot working. Did you verify that this all works correctly in this PR?

Also, large patchfiles as are included here are far from ideal and upstream likely has reasons do change things; so just reverting upstream commits is not something we should do hastily.

@barracuda156
Copy link
Contributor Author

@reneeotten Let’s do this PR together then? I make it a draft for now.

If you could implement meson backend either for python 3.12 (where it is perhaps needed) or for some subset of systems (we do not need meson backend for all systems, as of now, it seems to be rather defunct, judging by numpy, and legacy backend will be retained for pre-3.12 pythons anyway), if would be great.

As for patches you refer to, they are applied only for systems where legacysupport is used and only when the build is with gcc.
It is a known problem that our cmath does not work correctly, at least with gcc headers, but someone has to fix that – once it is done, it is easy to drop patches.
@catap Kirill, FYI.

@barracuda156 barracuda156 marked this pull request as draft December 21, 2023 00:56
@barracuda156
Copy link
Contributor Author

barracuda156 commented Dec 21, 2023

P. S. I did not test exotic BLAS variants. atlas will probably not build at all on new systems, no one implemented build with newer compilers so far, I believe.

By the way, blis appears to be supported: https://ports.macports.org/port/blis

@barracuda156
Copy link
Contributor Author

TODO:

  1. Perhaps, add build conflict with basiclu – otherwise one of installed dylibs may opportunistically link to it.
  2. Maybe support blis.
  3. Restrict altas to systems where it actually builds (?).

@erikbs erikbs mentioned this pull request Dec 22, 2023
@erikbs
Copy link
Contributor

erikbs commented Dec 22, 2023

meson backend either for python 3.12 (where it is perhaps needed)

I was also working on this port locally and can confirm that Meson is needed for Python 3.12. I tried adding a py312 subport to the existing setup.py-based port, but it failed because distutils is missing. Then I updated to 1.11.4, but it was the same (on master setup.py is gone). With Meson and a few patches it finally built. (As you might have noticed I pushed a now-closed PR after missing both this PR and the Trac ticket. Being used to typing “script” I accidentally searched for “scipt” instead of “scipy” and did not catch the mistake until after I had opened the PR …).

legacy backend will be retained for pre-3.12 pythons anyway

Does it have to? I mean, for this port. In my attempt to build with Meson I tried building the py311 subport as well, and it worked just fine (at least after replacing the numpy== requirements with numpy>= in pyproject.toml). And py-meson-python seems to be supported on older Mac versions.

I did not test exotic BLAS variants. atlas will probably not build at all on new systems

I use an older system where ATLAS is supported. If needed, I can help test the atlas variant. I did have trouble getting NumPy to use it when OpenBLAS was broken, so it would not surprise me if there are issues with SciPy too.

@catap
Copy link
Contributor

catap commented Dec 22, 2023

I did have trouble getting NumPy to use it when OpenBLAS was broken, so it would not surprise me if there are issues with SciPy too.

it should be fixed, as far as I know.

@barracuda156
Copy link
Contributor Author

@erikbs

Does it have to? I mean, for this port.

Well, recent switch to meson build for numpy essentially left it broken for older systems. It was also done without much care, which left a number of things in the port not working. So it had to be reverted to unbreak the port.
Possibly, with a recent fix from upstream, getting rid of xcrun bug, plus some fix for PowerPC recognizing, which meson-python fails to handle correctly, it will work with pep517. I did not try that yet.
There is no proper fix for PowerPC by the way. There is a way to make it work, or rather a few options to hack around broken meson, but neither is ideal. Some discussion is here, not very fruitful so far: mesonbuild/meson-python#553

So I suspect that scipy is broken in a similar way, and there is no point in switching from the build system which is known to work to an experimental thing which very likely will not work in a way we may think it will. For Python 3.12 it is a necessity, for the rest: what do we gain besides breakages?

@erikbs
Copy link
Contributor

erikbs commented Dec 23, 2023

I see. I was not aware of the problems related to Meson, I thought the NumPy issues only were consequences of the OpenBLAS issues after the switch to CMake. If the Meson solution is experimental, I agree that it is best to restrict it to only where it is needed. If it had been stable, I would think that dealing with one build system would be better than dealing with two (and give a cleaner Portfile).

@erikbs erikbs mentioned this pull request Dec 27, 2023
11 tasks
@barracuda156
Copy link
Contributor Author

@reneeotten We could have merged this first and fix the build, and then you could move new systems and python 3.12 to meson in another PR. What do you think?

Copy link
Contributor

@reneeotten reneeotten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barracuda156 if you could comment/address the few comments I left then I think we should merge this. @michaelld hasn't responded since the beginning of this PR and we could merge these changes to get the update on all currently supported versions.

Python 3.12 support is still much needed, but perhaps providing that separately for now with the meson backend is the quickest way forward. Nevertheless, with a newer release of SciPy we will have to bite the bullet and switch everything to the new backend as they will not be providing support for the setuptools-based installs and we should/will not keep on patching this in....

python/py-scipy/Portfile Show resolved Hide resolved
python/py-scipy/Portfile Outdated Show resolved Hide resolved
@barracuda156
Copy link
Contributor Author

barracuda156 commented Jan 5, 2024

@reneeotten I have moved tarball_from as you advised and dropped revbump for py38 version.

UPD. So this is why I revbumped py38- one: https://github.com/macports/macports-ports/actions/runs/7423947967?pr=21875
It fails without it.

@reneeotten
Copy link
Contributor

please add the Trac tickets to the body of the commit message and then I will merge. I will add vanilla support for PY312 (i.e., possibly without variants right now) and we will work on moving to the new backend completely for the next release.

If possible, please add fixing legacy-support high on your to-do list so that we don't have to carry these patches forward too long...

@reneeotten
Copy link
Contributor

@barracuda156 I just added the Trac tickets to the commit message but now, for some reason, it says "This branch cannot be rebased due to too many changes" - no idea how to fix that yet....

@barracuda156
Copy link
Contributor Author

I just added the Trac tickets to the commit message but now, for some reason, it says "This branch cannot be rebased due to too many changes" - no idea how to fix that yet....

@reneeotten It worked now, right? CI running, and commit body has links.

@reneeotten
Copy link
Contributor

probably a temporary GitHub glitch...

@barracuda156
Copy link
Contributor Author

barracuda156 commented Jan 5, 2024

Wonder what fails now, we literally changed nothing with the code besides tarball_from and reverted revbump (and CI already passed earlier).

@barracuda156
Copy link
Contributor Author

barracuda156 commented Jan 5, 2024

@reneeotten I guess, some recent changes to cython breaking py38-scipy subport?

  Cythonizing sources
  Traceback (most recent call last):
    File "setup.py", line 255, in generate_cython
      import pip
  ModuleNotFoundError: No module named 'pip'
  
  During handling of the above exception, another exception occurred:
  
  Traceback (most recent call last):
    File "setup.py", line 533, in <module>
      setup_package()
    File "setup.py", line 517, in setup_package
      generate_cython()
    File "setup.py", line 257, in generate_cython
      raise RuntimeError("Running cythonize failed!")
  RuntimeError: Running cythonize failed!
  Command failed:  cd "/opt/local/var/macports/build/_Users_runner_work_macports-ports_macports-ports_ports_python_py-scipy/py38-scipy/work/scipy-1.10.1" && /opt/local/Library/Frameworks/Python.framework/Versions/3.8/bin/python3.8 setup.py --no-user-cfg config_fc --fcompiler gnu95 --f77exec /opt/local/bin/gfortran-mp-13 --f77flags='-m64 -Os -fno-second-underscore' --f90exec /opt/local/bin/gfortran-mp-13 --f90flags='-m64 -Os -fno-second-underscore' config --cc /usr/bin/clang --include-dirs /opt/local/include --library-dirs /opt/local/lib build -j4 
  Exit code: 1
  Error: Failed to build py38-scipy: command execution failed

@reneeotten
Copy link
Contributor

Yes, there is a Trac ticket about it... Haven't had time to look at that - perhaps tonight. Perhaps we should use the earlier version for now (i.e., py-cython-compat).

@reneeotten
Copy link
Contributor

the last commit should hopefully fix it - it uses py-cython-compat for the PY38 subport which appears to be the problematic one here.... I'll merge if the CI passes.

@barracuda156
Copy link
Contributor Author

@reneeotten Great, thank you!

@reneeotten reneeotten merged commit a77e647 into macports:master Jan 6, 2024
3 checks passed
@barracuda156 barracuda156 deleted the scipy branch January 6, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6 participants