-
-
Notifications
You must be signed in to change notification settings - Fork 503
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
Use meson instead of configure for conda install #36489
Changes from 13 commits
bfac057
9694046
376742a
e270392
a21655b
84ff7ff
885e2b4
f755a8e
4406868
c0cdb5f
b408311
4c8b56b
f6f27d9
73e8c86
1b61d63
3ff66b8
6c78eaa
d9e0d49
7a44082
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,5 +34,6 @@ | |
"sagemath", | ||
"Cython" | ||
], | ||
"editor.formatOnType": true | ||
"editor.formatOnType": true, | ||
"esbonio.sphinx.confDir": "" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
project('sage', 'c') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be more comfortable with this file being local to sage-conf_meson, so as not to create expectations that Sage-the-distribution will switch to meson There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea in the middle to long term is to support running There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not aware of such a plan, and I'd suggest that if you have a plan to use a new Issue in which you explain it. From what you wrote above, it seems unlikely that it would be the same meson.build file that would be suitable for both purposes, so nothing is gained by putting it there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's part of #34630. In the long-term you want to use of course meson-python (configured via pyproject.toml). But in the short to midterm its easier to just call meson directly. That's at least how scipy/numpy proceeded: https://github.com/scipy/scipy/pull/14847/files#diff-e42998d51257e6f84aa51ffa5f4ed1544cb12f97a94978c44f3bc281b5324d79R390-R500 And yes, the meson file added in this PR can be used for this purpose. In fact, I believe we can get ride of the conf and setup packages, but of course there still needs to be some way to provide the runtime-config (probably by just putting the generated conf file in the correct place in the builddir). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, #34630 has nothing to with stuff that belongs in SAGE_ROOT. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There is nothing conda-specific about the meson files, it's only that conda provides a nice fixed environment and thus is a good starting point. You can try to run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll explain and summarize once more:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So if you see a hammer and a saw next to each other you are bound to see them as alternatives for building the same thing? Conceptually, It's also not technically possible to move the meson file in the root to Anyway, I'm tired of this stupid discussion and will stop participating in it. If you think that the harm created by the meson file in the root outweighs the positives of this PR, then please go ahead and continue blocking it. Please leave the "ready for review" tag on, as there are no work items that referee and PR creator agree on (it just that you don't like a small design choice in the PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The target There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually |
||
cc = meson.get_compiler('c') | ||
conf_data = configuration_data() | ||
|
||
conf_data.set('PACKAGE_VERSION', '1.2.3') | ||
|
||
maxima = find_program('maxima', required: true) | ||
conf_data.set('SAGE_MAXIMA', maxima.path()) | ||
# Conda's ecl does not have any problems with Maxima, so nothing needs to be set here: | ||
conf_data.set('SAGE_MAXIMA_FAS', '') | ||
|
||
# Kenzo cannot yet be provided by the system, so we always use the SAGE_LOCAL path for now. | ||
conf_data.set('SAGE_KENZO_FAS', '\'${prefix}\'/lib/ecl/kenzo.fas') | ||
|
||
conf_data.set('SAGE_ARB_LIBRARY', 'arb') | ||
|
||
#ntl = dependency('ntl', required: true) doesn't work, so ask the compiler directly: | ||
ntl = cc.find_library('ntl', required: true) | ||
# It can be found, so we don't have to set anything here: | ||
conf_data.set('NTL_INCDIR', '') | ||
conf_data.set('NTL_LIBDIR', '') | ||
|
||
ecl_config = find_program('ecl-config', required: true) | ||
conf_data.set('SAGE_ECL_CONFIG', ecl_config.path()) | ||
|
||
conf_data.set('SAGE_ARCHFLAGS', 'unset') | ||
|
||
# not needed when using conda, as we then don't build any pc files | ||
conf_data.set('SAGE_PKG_CONFIG_PATH', '') | ||
|
||
openmp = dependency('openmp', required : false) | ||
if openmp.found() | ||
conf_data.set('OPENMP_CFLAGS', '-fopenmp') | ||
conf_data.set('OPENMP_CXXFLAGS', '-fopenmp') | ||
endif | ||
|
||
configure_file(input : 'pkgs/sage-conf_conda/_sage_conf/_conf.py.in', | ||
output : '_conf.py', | ||
configuration : conf_data) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,10 @@ | ||
[build-system] | ||
requires = ["setuptools", "wheel"] | ||
build-backend = "setuptools.build_meta" | ||
|
||
[project] | ||
name = "sage-conf" | ||
dynamic = ["version"] | ||
|
||
[tool.setuptools.dynamic] | ||
version = {file = ["VERSION.txt"]} | ||
tobiasdiez marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/_sage_conf/_conf.py | ||
/builddir | ||
/build | ||
/dist | ||
/*.egg-info | ||
/.tox | ||
/bin/sage-env-config |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../sage-conf/README.rst |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
10.2.beta7 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../sage-conf/_sage_conf |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../sage-conf/pyproject.toml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from _sage_conf.__main__ import _main | ||
|
||
from builddir._conf import * | ||
from builddir.build_info import CONDA_PREFIX, SAGE_ROOT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This "from builddir... import" makes me a bit uncomfortable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I knew that when I wrote the above. So? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the difference between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In https://github.com/sagemath/sage/blob/develop/pkgs/sage-conf/sage_conf.py#L1 we have:
Here:
More questions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You may be right about this, but a lot currently still depends on the assumption "$SAGE_LOCAL = $CONDA_PREFIX" (e.g., see #30914 for pointers), which is guaranteed by the all-conda installation instructions, but I am not sure that we will want to keep it in the future. A problem with this assumption is that we currently have an inflexible either-or situation:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, by design it's required, but as is explained in https://github.com/sagemath/sage/blob/develop/pkgs/sage-conf/README.rst, you can pick your own implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, but in the "we take everything from conda" the assumption will always be valid, right? So in this case we don't need a sage_conf package. If in the future some semi-mixed versions are supported, then this method needs its own sage_conf package.
This document doesn't say anything on why we need the package nor why the design cannot be changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either way, I've fixed the package now. Please give it a try. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My point is that this dichotomy of the two installation methods is not sustainable; so it would be unwise to be invested in a "pure" mode that seeks to eliminate sage_conf altogether. |
||
|
||
SAGE_LOCAL = CONDA_PREFIX |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
import os | ||
import sys | ||
from distutils.command.build_scripts import \ | ||
build_scripts as distutils_build_scripts | ||
from pathlib import Path | ||
|
||
from setuptools import setup | ||
from setuptools.command.build_py import build_py as setuptools_build_py | ||
from setuptools.command.editable_wheel import \ | ||
editable_wheel as setuptools_editable_wheel | ||
from setuptools.errors import SetupError | ||
|
||
|
||
class build_py(setuptools_build_py): | ||
def run(self): | ||
here = Path(__file__).parent | ||
if self.editable_mode: | ||
root = here.parent.parent | ||
else: | ||
raise SetupError('Not supported') | ||
|
||
conda_prefix = os.environ.get('CONDA_PREFIX', '') | ||
if not conda_prefix: | ||
raise SetupError( | ||
'No conda environment is active. ' | ||
'See https://doc.sagemath.org/html/en/installation/conda.html on how to get started.' | ||
) | ||
|
||
builddir = here / "builddir" | ||
cmd = f"cd {root} && meson setup {builddir} --wipe" | ||
print(f"Running {cmd}") | ||
sys.stdout.flush() | ||
if os.system(cmd) != 0: | ||
raise SetupError("configure failed") | ||
|
||
# Write build info | ||
with open(builddir / 'build_info.py', 'w', encoding="utf-8") as build_info: | ||
build_info.write(f'SAGE_ROOT = "{root}"\n') | ||
build_info.write(f'CONDA_PREFIX = "{conda_prefix}"\n') | ||
|
||
|
||
class build_scripts(distutils_build_scripts): | ||
def run(self): | ||
self.distribution.scripts.append(os.path.join('bin', 'sage-env-config')) | ||
if not self.distribution.entry_points: | ||
self.entry_points = self.distribution.entry_points = dict() | ||
distutils_build_scripts.run(self) | ||
|
||
|
||
class editable_wheel(setuptools_editable_wheel): | ||
r""" | ||
Customized so that exceptions raised by our build_py | ||
do not lead to the "Customization incompatible with editable install" message | ||
""" | ||
_safely_run = setuptools_editable_wheel.run_command | ||
|
||
|
||
setup( | ||
cmdclass=dict( | ||
build_py=build_py, build_scripts=build_scripts, editable_wheel=editable_wheel | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change do? Does it have a relation to the goal of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vscode is adding this every time I'm opening the project. I was bound to commit it at some point ;-). I doesn't hurt...