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

Compile everything with meson #36524

Merged
merged 283 commits into from
Oct 26, 2024
Merged

Compile everything with meson #36524

merged 283 commits into from
Oct 26, 2024

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Oct 24, 2023

Add meson configuration to compile sagelib with meson. Basic developer docs are added as well (meant as background info, not real installation instructions and thus under "developer").

📝 Checklist

  • 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

@tobiasdiez tobiasdiez changed the title Meson-compile Compile categories with meson Oct 24, 2023
@tobiasdiez tobiasdiez marked this pull request as ready for review October 24, 2023 03:20
@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 24, 2023

Note that the sage.categories namespace package is not shipped by one distribution, but rather split into two distribution packages - sagemath-objects and sagemath-categories.

@tobiasdiez
Copy link
Contributor Author

Note that the sage.categories namespace package is not shipped by one distribution, but rather split into two distribution packages - sagemath-objects and sagemath-categories.

I don't see how this is relevant here. I'm trying to build the full sagelib with meson. Just started with a small part to make reviewing easier

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 24, 2023

Just a heads up that the monolithic build is going to go away very soon. So it would be unwise to design a new system for the monolithic library.

@tornaria tornaria mentioned this pull request Oct 25, 2023
3 tasks
@tobiasdiez tobiasdiez changed the title Compile categories with meson Compile almost everything with meson Oct 31, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 23, 2024
    
<!-- ^^^^^
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? -->
<!-- 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. -->

Add meson configuration to compile sagelib with meson. Basic developer
docs are added as well (meant as background info, not real installation
instructions and thus under "developer").

### 📝 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.
- [x] 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#36524
Reported by: Tobias Diez
Reviewer(s): Dima Pasechnik, Gonzalo Tornaría, Matthias Köppe, Michael Orlitzky, Tobias Diez
@vbraun vbraun merged commit 61e1f4a into sagemath:develop Oct 26, 2024
22 of 23 checks passed
@antonio-rojas
Copy link
Contributor

Testing this in 10.5.beta8, I get a compile error:

src/sage/libs/coxeter3/coxeter.cpython-312-x86_64-linux-gnu.so.p/src/sage/libs/coxeter3/coxeter.pyx.cpp:2985:10: fatal error: string_impl.h: No such file or directory
 2985 | #include "string_impl.h"
      |          ^~~~~~~~~~~~~~~
compilation terminated.

The usual setup.py compilation works fine.

@dimpase
Copy link
Member

dimpase commented Oct 26, 2024

optional packages aren't done yet. in a follow-up.

@antonio-rojas
Copy link
Contributor

Ahother issue: there is a duplicate sage/config.py entry in the generated wheel, so python installer fails to install it

sage/config.py,sha256=nHmbVg0MfUpLwYm1Xh2bQ74etHfY_SuPBnyAZwMnJ5k,2337
sage/config.py,sha256=nHmbVg0MfUpLwYm1Xh2bQ74etHfY_SuPBnyAZwMnJ5k,2337
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/usr/lib/python3.12/site-packages/installer/__main__.py", line 98, in <module>
    _main(sys.argv[1:], "python -m installer")
  File "/usr/lib/python3.12/site-packages/installer/__main__.py", line 94, in _main
    installer.install(source, destination, {})
  File "/usr/lib/python3.12/site-packages/installer/_core.py", line 109, in install
    record = destination.write_file(
             ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/installer/destinations.py", line 207, in write_file
    return self.write_to_fs(scheme, path_, stream, is_executable)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/installer/destinations.py", line 167, in write_to_fs
    raise FileExistsError(message)
FileExistsError: File already exists: /build/sagemath-git/pkg/sagemath-git/usr/lib/python3.12/site-packages/sage/config.py

@antonio-rojas
Copy link
Contributor

optional packages aren't done yet. in a follow-up.

well, the build system tries to build them anyway

@dimpase
Copy link
Member

dimpase commented Oct 26, 2024

I haven't seen this. How did you do the meson build?

@dimpase
Copy link
Member

dimpase commented Oct 26, 2024

Ahother issue: there is a duplicate sage/config.py entry in the generated wheel, so python installer fails to install it

sage/config.py,sha256=nHmbVg0MfUpLwYm1Xh2bQ74etHfY_SuPBnyAZwMnJ5k,2337
sage/config.py,sha256=nHmbVg0MfUpLwYm1Xh2bQ74etHfY_SuPBnyAZwMnJ5k,2337
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/usr/lib/python3.12/site-packages/installer/__main__.py", line 98, in <module>
    _main(sys.argv[1:], "python -m installer")
  File "/usr/lib/python3.12/site-packages/installer/__main__.py", line 94, in _main
    installer.install(source, destination, {})
  File "/usr/lib/python3.12/site-packages/installer/_core.py", line 109, in install
    record = destination.write_file(
             ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/installer/destinations.py", line 207, in write_file
    return self.write_to_fs(scheme, path_, stream, is_executable)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/installer/destinations.py", line 167, in write_to_fs
    raise FileExistsError(message)
FileExistsError: File already exists: /build/sagemath-git/pkg/sagemath-git/usr/lib/python3.12/site-packages/sage/config.py

again, it's not clear without more details how to reproduce this.

I tested by creating a standard venv (no isolation) and ran pip install there. Then ./sage -t to test

@antonio-rojas
Copy link
Contributor

I haven't seen this. How did you do the meson build?

python -m build --wheel --no-isolation

@dimpase
Copy link
Member

dimpase commented Oct 27, 2024

well, that's a different toolchain. It should work, though, too. Open an issue?

@dimpase
Copy link
Member

dimpase commented Oct 27, 2024

an advantage of meson build as tested is on the fly rebuild of changed pieces, cython too. Wheel building is different, we should test and fix it.

@antonio-rojas
Copy link
Contributor

There's a regression where installed rst files are no longer doctested, fixed in #38863

@antonio-rojas
Copy link
Contributor

antonio-rojas commented Oct 27, 2024

Opened #38866 and #38867 fixing the coxeter3 extension build and the duplicate config.py issues.

@tobiasdiez
Copy link
Contributor Author

This has eaten several merge commits over the past year, and has lots of tiny back-and-forth commits, as well as some things that were initially controversial but have since been resolved. The git diff develop isn't bad, but trying to figure anything out from github is impossible.

I certainly won't stop you, but I think it would be much nicer to rebase this whole thing as a series of <15 commits with decent commit messages (feel free to squash away my few commits). The meson.build files, for example, could be one commit instead of a hundred. Messing with the CI another. Necessary test fixes a third, etc.

Sorry only saw this now. Cleaning up the git history would have been nice indeed. I guess its too late now :(

@tobiasdiez
Copy link
Contributor Author

Opened #38866 and #38867 fixing the coxeter3 extension build and the duplicate config.py issues.

Thanks for giving it a test run and for providing these fixes!

@tobiasdiez tobiasdiez deleted the meson-compile branch October 28, 2024 02:00
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 2, 2024
    
Fix a regression from sagemath#36524 where
`sage -t` skips doctesting installed `rst` files (which get installed as
`rst.txt`)
    
URL: sagemath#38863
Reported by: Antonio Rojas
Reviewer(s): Tobias Diez
@tornaria
Copy link
Contributor

tornaria commented Nov 7, 2024

@tobiasdiez it seems that make sagelib-clean will remove src/sage/ext/interpreters/meson.build. Maybe we need something like:

--- a/Makefile
+++ b/Makefile
@@ -122,7 +122,9 @@ sagelib-clean:
             rm -rf c_lib .cython_version cython_debug; \
             rm -rf build; find . -name '*.pyc' -o -name "*.so" | xargs rm -f; \
             rm -f $$(find . -name "*.pyx" | sed 's/\(.*\)[.]pyx$$/\1.c \1.cpp/'); \
-            rm -rf sage/ext/interpreters) \
+            rm -f sage/ext/interpreters/interp_*.c ; \
+            rm -f sage/ext/interpreters/wrapper_*.p* ; \
+            rm -f sage/ext/interpreters/all.py ) \
            && (cd "$(SAGE_ROOT)/build/pkgs/sagelib/src/" && rm -rf build); \
        fi
 

@user202729
Copy link
Contributor

user202729 commented Nov 8, 2024

Meanwhile can the README also be updated to include instruction how to build with meson, [edit: looks like it's at https://doc-release--sagemath.netlify.app/html/en/installation/meson . Just not released yet] and SageMath developer guide updated on how to modify meson.build for each file added? At the moment it can be somewhat unclear…

@tobiasdiez
Copy link
Contributor Author

SageMath developer guide updated on how to modify meson.build for each file added? At the moment it can be somewhat unclear…

What exactly are you missing? Would it be enough to say that new python files should be added to the meson.build in the same folder?

@user202729
Copy link
Contributor

Sounds about right, but actually since people may just not read the documentation, why can't we just glob the folder for all .py and .pyx files?

(And meanwhile why is the determination of compiling with C versus C++ implicit in Makefile and need to be specifically specified in meson?)

@dimpase
Copy link
Member

dimpase commented Nov 11, 2024

(And meanwhile why is the determination of compiling with C versus C++ implicit in Makefile and need to be specifically specified in meson?)

that's because meson does not support getting this info from special comments in .pyx files.

@tobiasdiez
Copy link
Contributor Author

Sounds about right, but actually since people may just not read the documentation, why can't we just glob the folder for all .py and .pyx files?

Specifying source files by glob-patterns is just not supported by meson, and for good reasons: https://mesonbuild.com/FAQ.html#why-cant-i-specify-target-files-with-a-wildcard
(This is by the way not specific to meson, cmake also strongly encourages to explicitly list the source files).

@user202729
Copy link
Contributor

Specifying source files by glob-patterns is just not supported by meson, and for good reasons: https://mesonbuild.com/FAQ.html#why-cant-i-specify-target-files-with-a-wildcard

Makes sense.

For my purpose (developer accidentally forget to include file in meson build file and get puzzled over error) I think it would be a good thing (if it can be implemented) to somehow only check that there's no file on disk that is not listed in meson.build in e.g. a full build, or on the CI.

Then the performance concerns are not a problem.

@orlitzky
Copy link
Contributor

Now that I've had more time to spend on it, I'm using

meson setup --prefix="${HOME}/.local"

so that the result is installed to my default PYTHONPATH and I can use it from python without any further tricks.

To run the sage repl I have a script (called sage, on my PATH),

#!/bin/sh

# This is the default when the variable is unset (or empty), but the
# "sage" script will clobber it in that case, so we have to explicitly
# set it to the default value.
export PYTHONUSERBASE="${HOME}/.local"

# Needed to find the sage-on-gentoo database packages.
export SAGE_SHARE="/usr/share/sage"

exec "${HOME}/src/sage.git/sage" "${@}"

The last line just points to the usual "sage" script, until I find a better way to do that. And the SAGE_SHARE may not apply to you. Eventually we will fix those stupid database packages.

@antonio-rojas
Copy link
Contributor

Is it intentional that this only installs the python library, and not any of the executable scripts? (sorry if this has been addressed before, I only skimmed through the 200+ posts)

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.

10 participants