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

meson on windows with clang-cl & flang; python 3.12 support #255

Merged
merged 21 commits into from
Oct 5, 2023

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Sep 29, 2023

This is a cleaned-up version of #246, taking into account learnings & patches contributed by several people (also in #252 & #253), some of which were directly upstreamed to scipy and made it into 1.11.3 already.

Requires builds from conda-forge/flang-feedstock#28 & conda-forge/meson-feedstock#89 conda-forge/meson-feedstock#92 which are currently only available under llvm_rc and meson_dev labels. This will hopefully improve soon.

It's likely that this will still need some work around compiler-activation, flags, etc.; but with a passing test suite in #246, we should be good for more in-depth review.

Closes #253
Closes #252
Closes #246
Closes #213
Closes #164

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

Don't let the mood be spoiled by the pypy failure, it happens similarly on main and is due to conda-forge/pypy3.6-feedstock#109. ;-)

Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

The recipe changes look quite good to me. I'll approve based on what I understand. The two things I can't judge are:

  1. The use of labels (conda-forge/label/llvm_rc,conda-forge/label/meson_dev,conda-forge)
  2. The Windows-specific flags in bld.bat

@h-vetinari
Copy link
Member Author

h-vetinari commented Sep 29, 2023

The use of labels

It's not ideal, but both are "only" build tools, and generate no run-time dependence for scipy from a non-main channel, so I'm not really worried about that part (though of course I'm trying to get rid of these labels by finishing up flang and meson support).

The Windows-specific flags in bld.bat

This is one thing where I'd like to have an in-depth review. The fact that the test suite passes is already a good start, but there are many ABI-relevant pieces at play here (clang vs. clang-cl, link.exe vs. lld-link, compiler-rt vs. msvcrt vs. libcmt, flang vs. all of the above, etc.).

In particular, we might need to switch some of the flags to MSVC-syntax so that clang-cl will take them. Some of that could eventually also move to the clang_win-64 activation1; in any case, I'm hoping @isuruf, @xhochy et al. will be able to chime in.

Footnotes

  1. I don't really see the point to have an activation for clang.exe, which makes clang_win-64 effectively unusable (or worse: dangerous to use) in conda-forge. Might as well activate for clang-cl.exe...?

@h-vetinari
Copy link
Member Author

No idea why meson would not pick up flang specifically on python 3.12 - those things should be completely unrelated? 🤔

..\meson.build:82:0: ERROR: Unknown compiler(s): [['flang-new']]
The following exception(s) were encountered:
Running `flang-new --version` gave "[WinError 2] The system cannot find the file specified"
Running `flang-new -V` gave "[WinError 2] The system cannot find the file specified"

@h-vetinari h-vetinari changed the title meson on windows with clang-cl & flang meson on windows with clang-cl & flang; python 3.12 support Oct 1, 2023
@mattip
Copy link
Contributor

mattip commented Oct 1, 2023

@conda-forge-admin, please restart ci

@mattip
Copy link
Contributor

mattip commented Oct 1, 2023

Whoops, sorry @h-vetinari. Our attempts crossed wires.

recipe/bld.bat Outdated Show resolved Hide resolved
recipe/bld.bat Show resolved Hide resolved
Copy link
Member Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Thanks for the review! Flang doesn't understand --dependent-lib as an option yet, so I'm not sure what we should do with that for now.

recipe/bld.bat Show resolved Hide resolved
recipe/bld.bat Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

h-vetinari commented Oct 1, 2023

OK, so it seems we were picking up some symbols from compiler-rt after all (this from an attempt to remove compiler-rt as a host dep, and referring to it in LDFLAGS):

[259/1628] Linking target scipy/special/_specfun.cp310-win_amd64.pyd
FAILED: scipy/special/_specfun.cp310-win_amd64.pyd 
"flang-new"  -Wl,/OUT:scipy/special/_specfun.cp310-win_amd64.pyd scipy/special/_specfun.cp310-win_amd64.pyd.p/meson-generated_..__specfunmodule.c.obj "-LC:/bld/scipy-split_1696193939756/_build_env/Library/lib/clang/17" "-Wl,/nologo" "-Wl,/OPT:REF" "-Wl,/DLL" "-Wl,/IMPLIB:scipy\special\_specfun.cp310-win_amd64.lib" "--target=x86_64-pc-windows-msvc" "-nostdlib" "-Xclang" "--dependent-lib=msvcrt" "-fuse-ld=lld" "-D_CRT_SECURE_NO_WARNINGS" "-D_MT" "-D_DLL" "--target=x86_64-pc-windows-msvc" "-nostdlib" "scipy\special\libspecfun.a" "scipy\lib_fortranobject.a" "%PREFIX%\libs\python310.lib" "-lFortranRuntime" "-lFortranDecimal"
lld-link: error: undefined symbol: __divdc3

>>> referenced by libspecfun.a(specfun_specfun.f.obj):(cpdsa_)
>>> referenced by libspecfun.a(specfun_specfun.f.obj):(cpdsa_)
>>> referenced by libspecfun.a(specfun_specfun.f.obj):(cfs_)
>>> referenced 318 more times

I couldn't find __divdc3 for msvc anywhere (in docs online, or in a local SDK), so I think we'll have to actually stay on compiler-rt here.

Copy link
Member Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

@isuruf, I think I've addressed all your points (short of patching upstream flang to process --dependent-lib...)

Do you have any remaining concerns here?

recipe/bld.bat Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

Do you have any remaining concerns here?

Gentle ping @isuruf :)

recipe/bld.bat Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

Took the opportunity to rebase out things we don't need anymore to reduce the diff a bit:

@h-vetinari
Copy link
Member Author

Link checks are tidy now and the windows builds are green; given that the latter was the key change here, I'm going to put this in.

Thanks for the help everyone! What a journey it's been! 😅 🚀

@h-vetinari h-vetinari merged commit ccf7ef7 into conda-forge:main Oct 5, 2023
19 of 28 checks passed
@h-vetinari h-vetinari deleted the win_meson_clean branch October 5, 2023 00:58
@traversaro
Copy link

Thanks a lot @h-vetinari for the great effort on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Meson integration on windows - discussion about compiler stack Proposed build system changes to SciPy
5 participants