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

pkgs/*/setup.cfg: Reduce boilerplate by m4-include, reduce complexity of m4 use #35867

Merged
merged 8 commits into from
Jul 9, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jun 30, 2023

πŸ“š Description

This makes the pkgs/*/setup.cfg.m4 and pkgs/*/pyproject.toml.m4 files less scary and less repetitive.

Split out from:

πŸ“ 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

@mkoeppe mkoeppe self-assigned this Jun 30, 2023
@github-actions
Copy link

Documentation preview for this PR (built with commit 5ecf106; changes) is ready! πŸŽ‰

@dimpase
Copy link
Member

dimpase commented Jul 1, 2023

This pytest-related error is everywhere in CI output, it seems

============================ test session starts ==============================
platform linux -- Python 3.8.10, pytest-7.3.2, pluggy-1.0.0
rootdir: /sage
plugins: mock-3.11.1
collected 0 items / 1 error

==================================== ERRORS ====================================
________________________ ERROR collecting test session _________________________
/usr/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1014: in _gcd_import
    ???
<frozen importlib._bootstrap>:991: in _find_and_load
    ???
...

@dimpase
Copy link
Member

dimpase commented Jul 1, 2023

Can it be fixed by dropping Python 3.8 ?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 1, 2023

This error is new, from the incremental build introduced in #35652. Unrelated to the present PR and to unrelated to the Python version. Tracked in:

@mkoeppe mkoeppe requested a review from kiwifb July 7, 2023 17:43
@kiwifb
Copy link
Member

kiwifb commented Jul 7, 2023

Not sure about less scary, but factorisation of the repeated bits is good. It is much more easy to understand I think. What makes the build/test fail?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 7, 2023

What makes the build/test fail?

An occurrence of #35813, unrelated

@kiwifb
Copy link
Member

kiwifb commented Jul 7, 2023

I understand what Dima was talking about now :) Looks good to me.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 7, 2023

Thanks! May I set to "positive review"?

@vbraun vbraun merged commit e3c11b6 into sagemath:develop Jul 9, 2023
14 of 15 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 9, 2023
@tornaria
Copy link
Contributor

After this was merged I can't bootstrap sagelib on its own.

This works in 10.0.beta5:

$ cd build/pkgs/sagelib
$ PATH=../../bin:$PATH ./bootstrap

After bootstraping sagelib like that one can build sagemath-standard on its own and it works perfectly well (I've been doing it for the void linux package since 9.5).

Same thing in 10.0.beta6:

$ cd build/pkgs/sagelib
$ PATH=../../bin:$PATH ./bootstrap)
m4:src/setup.cfg.m4:1: cannot open `sage_spkg_versions.m4': No such file or directory
m4:src/setup.cfg.m4:9: cannot open `setup_cfg_metadata.m4': No such file or directory

Half of this is due to SAGE_ROOT missing so M4PATH is not correctly set. But still:

$ SAGE_ROOT=../../.. PATH=../../bin:$PATH ./bootstrap
m4:src/setup.cfg.m4:1: cannot open `sage_spkg_versions.m4': No such file or directory

Now the problem is that m4/sage_spkg_versions.m4 is not available and the only way to create it seems to be running $SAGE_ROOT/bootstrap which wants to bootstrap the whole sage-the-distro I don't need or want at all (for one, it needs autotools which is not otherwise needed to build sagelib, and worse it will take minutes vs. less than 1s).

Actually creating m4/sage_spkg_versions.m4 is done early and it's very quick. Can you provide a standard simple way to create it on its own? More importantly, could we actually support a standard way to bootstrap just sagelib / sagemath-standard and build it on its own?

@dimpase
Copy link
Member

dimpase commented Jul 13, 2023 via email

@tornaria
Copy link
Contributor

I don't think inventing workarounds to avoid using autotools is very useful way to spend our time :-(

This has nothing to do with autotools, which is not used or needed to build sagemath-standard.

This PR broke the bootstrap script in build/pkgs/sagelib without offerning any alternative.

What is the point of modularization if we don't support the most basic split, namely at the boundary between sage the library and distribution packages.

@dimpase
Copy link
Member

dimpase commented Jul 13, 2023

This PR broke the bootstrap script in build/pkgs/sagelib without offerning any alternative.

No alternative? Why, you run the main ./bootstrap, if you must, or use pre-built configure spkg.

I don't think modularisation at the moment means that parts of the source tree become independent on each other - it's still one tree.

@mkoeppe mkoeppe deleted the m4_simplify branch July 13, 2023 14:51
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 13, 2023

By design, the various source trees become self-contained after running bootstrap.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 13, 2023

Actually creating m4/sage_spkg_versions.m4 is done early and it's very quick. Can you provide a standard simple way to create it on its own?

Yes, I think some refactoring of the top-level bootstrap script will be useful. I'm also annoyed by bootstrap taking too long. I'll look into it.

@tornaria
Copy link
Contributor

Actually creating m4/sage_spkg_versions.m4 is done early and it's very quick. Can you provide a standard simple way to create it on its own?

Yes, I think some refactoring of the top-level bootstrap script will be useful. I'm also annoyed by bootstrap taking too long. I'll look into it.

Since there is a Makefile maybe some of these bootstrap steps could be handled by that?

E.g. make bootstrap for which a step could be make bootstrap-sagelib using make m4/sage_spkg_versions.m4, etc.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 13, 2023

No, I definitely want to keep the bootstrap phase separate from the build system of the Sage distribution.

@mkoeppe mkoeppe mentioned this pull request Jul 13, 2023
5 tasks
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 13, 2023

@tornaria please take a look if #35950 goes in the right direction for you

@kiwifb
Copy link
Member

kiwifb commented Jul 13, 2023

Yes, I had the problem after the merge and I had myself to blame because I positively reviewed it. What really kills our old non-autotools way of boostraping sagemath-standard is that two generated .m4 files are missing without running the top bootstrap.

But, and I want to stress that out, this is a problem only if you build from the dev branch - mostly via cloning. For stable releases, I build from the tarballs on pypi which are already bootstrapped. While it is annoying and non-minimalistic (and I tried to extract a minimum process for a few hours but it always ended broken in strange way) I accepted that running autoconf for something I pull from git is acceptable. I would need to do it for any autotooled package I pull from git out there. Of course it is weird for what is supposed to be python package.

@kiwifb
Copy link
Member

kiwifb commented Jul 13, 2023

I also still do not run configure for people who wonder. While my attempt to generate only m4/sage_spkg_versions.m4 and m4/sage_spkg_versions_toml.m4 failed, I suspect that the way we build sagemath-standard in distro, providing empty files (or some minimal file with a bit of formatting) would be enough to restore our old way of bootstraping only build/pkg/sagelib.

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.

None yet

5 participants