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

upgrade Maxima to 5.46.0 #35619

Merged
merged 28 commits into from
May 28, 2023
Merged

upgrade Maxima to 5.46.0 #35619

merged 28 commits into from
May 28, 2023

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented May 6, 2023

📚 upgrade Maxima to 5.46.0

Our 5.45.0 is getting old. This will fix #32898 as well.

We have one mild regression found while working on #35615 (already reported upstream),
but number of improvements in a number of places. As #35615 is a part of this PR branch,
I propose to review them all here.

We also promote info spkg to standard, and make it install makeinfo/texi2any, to deal with Maxima docs.
This allows us to simplify configuration/installation of Maxima and ECL.

📝 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

#35615 - provide spkg-configure.m4 for Maxima

@dimpase
Copy link
Member Author

dimpase commented May 6, 2023

CI fails due to a demand for makeinfo (something new for 5.46, I gather):

2023-05-06T19:48:36.7969163Z [maxima-5.46.0] checking for ecl... true
2023-05-06T19:48:36.7969636Z [maxima-5.46.0] checking for makeinfo... false
2023-05-06T19:48:36.7970184Z [maxima-5.46.0] configure: error: No makeinfo found; consider --disable-build-docs

@dimpase
Copy link
Member Author

dimpase commented May 6, 2023

sage -t --random-seed=191226560908718331502074978913271091124 sage/tests/cmdline.py
**********************************************************************
File "sage/tests/cmdline.py", line 576, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    ''
Got:
    ';;; Warning: Maxima is unable to set up the help system.\n(Details: CL-INFO::LOAD-PRIMARY-INDEX: Filesystem error with pathname #P"/sage/local/share/info/maxima-index.lisp".\nEither\n 1) the file does not exist, or\n 2) we are not allowed to access the file, or\n 3) the pathname points to a broken symbolic link.)\n\n'

seems that we can't proceed without makeinfo...

@dimpase
Copy link
Member Author

dimpase commented May 8, 2023

OK, so we need to decide what to do with failing docs/help thing. The old hack, to avoid calling makeinfo, touching this and that, does not work any more (and build breaks due to lack of makebuild), maxima's configure option --disable-build-docs results in the build proceeding, but not providing the maxima help system.

The cleanest way out for me is to promote info spkg to standard, and let it build makeinfo, not only info.
Then all these hacks are no longer needed, as well as makeinfo version check in ECL's spkg-install.m4 - the latter will be moved to info's spkg-configure.m4.

Are you OK with it? If so, I'll go ahead with it. If not, please propose an alternative.

@dimpase
Copy link
Member Author

dimpase commented May 8, 2023

@kiwifb - in particular, your input would be valuable, as you appear to have written the touch hacks to avoid building docs..

@kiwifb
Copy link
Member

kiwifb commented May 8, 2023

The hack was really an unwillingness from anyone to require and then have makeinfo in the sage tree. If we bit that bullet, it should be fine. Plus, reading from the notes, the makeinfo provided by OS X used to be too old. Hopefully, this is not the case anymore, but someone with a mac should double check.
From memory there are a few doctests to fix or rewrite beyond the one in the current branch.

@dimpase
Copy link
Member Author

dimpase commented May 8, 2023

Plus, reading from the notes, the makeinfo provided by OS X used to be too old.

it seems that Apple stopped providing makeinfo all together. Anyway, while working on an ECL update I wrote tests to check for makeinfo to be new enough, and we can reuse them.

@dimpase
Copy link
Member Author

dimpase commented May 8, 2023

unwillingness from anyone to require and then have makeinfo in the sage tree. If we bit that bullet, it should be fine.

need to build (make)info would be limited to purists willing to build everything in Sage from scratch. Surely makeinfo and info is available in any sane building environment.

@kiwifb
Copy link
Member

kiwifb commented May 8, 2023

I vaguely remember working with David Kirkby on this. His solaris environment was not really standard. And because the object were already present, the other unwillingness was to waste the time rebuilding them. In any case, we have moved to a point where asking for makeinfo is not onerous. Let us move on.

@dimpase
Copy link
Member Author

dimpase commented May 8, 2023

unwillingness from anyone to require and then have makeinfo in the sage tree. If we bit that bullet, it should be fine.

The update is being tested now. The need to build (make)info would be limited to purists willing to build everything in Sage from scratch. Surely makeinfo and info are available in any sane building environment.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2023

+1 on making the info spkg standard and building makeinfo.

@dimpase
Copy link
Member Author

dimpase commented May 8, 2023

OK, so this test run only returns the test errors coming from 5.46 being different from 5.45. Will be fixed later today.
{{{
sage -t --random-seed=312649298022371725183055163141926694063 sage/calculus/calculus.py # 5 doctests failed
sage -t --random-seed=312649298022371725183055163141926694063 sage/functions/exp_integral.py # 1 doctest failed
sage -t --random-seed=312649298022371725183055163141926694063 sage/interfaces/maxima_lib.py # 2 doctests failed
sage -t --random-seed=312649298022371725183055163141926694063 sage/symbolic/integration/integral.py # 1 doctest failed
sage -t --random-seed=312649298022371725183055163141926694063 sage/symbolic/relation.py # 1 doctest failed
}}}

@kiwifb
Copy link
Member

kiwifb commented May 8, 2023

The test failures are all in #33718 along with various discussion including link to upstream bug report. It is interesting reading and not without reminding of a recent thread about treatment of number on sage-devel.

@orlitzky
Copy link
Contributor

orlitzky commented May 8, 2023

Two of the failures on #33718 look like bad tests to begin with. For example instead of testing that the string representation of 2*y*arctan(1.0/y) - 2*y*arctan(0.0001414/y) + 1.0*log(1.0*y^2 + 1.0) - 0.0001414*log(1.0*y^2 + 1.9993959999999997e-08) - 1.9997172 never changes, we could instead put the "expected" answer in expected, the actual one in actual, and then check that (expected - actual)(y=some number) is zeroish.

@dimpase
Copy link
Member Author

dimpase commented May 9, 2023

OK, all ready now. To accommodate both 5.45 and 5.46, a couple of tests had to be made a little weaker, because
we don't have a proper way to version tests, depending on a component version.

@dimpase
Copy link
Member Author

dimpase commented May 9, 2023

Two of the failures on #33718 look like bad tests to begin with. For example instead of testing that the string representation of 2*y*arctan(1.0/y) - 2*y*arctan(0.0001414/y) + 1.0*log(1.0*y^2 + 1.0) - 0.0001414*log(1.0*y^2 + 1.9993959999999997e-08) - 1.9997172 never changes, we could instead put the "expected" answer in expected, the actual one in actual, and then check that (expected - actual)(y=some number) is zeroish.

I've changed this test to use the exact interval, then it's uniform across the versions.

@dimpase dimpase mentioned this pull request May 9, 2023
@dimpase dimpase added the s: run conda ci Run the conda workflow on this PR. label May 9, 2023
@dimpase
Copy link
Member Author

dimpase commented May 9, 2023

@isuruf Conda runs here fail due to a conda package conflict involving different OpenSSL versions. Just in case - you probably saw this already.

@dimpase
Copy link
Member Author

dimpase commented May 22, 2023

I see the following on fedora-39 (https://github.com/mkoeppe/sage/actions/runs/5039255345/jobs/9037285404#step:13:10763)

if ecl comes from the system, then info might not be there yet.

hopefully fixed by cc522fa

Also, for some reason, texinfo is not in the list of the system packages being installed in the GH Actions run in question,
I don't know why.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 22, 2023

if ecl comes from the system, then info might not be there yet.

No, that's not what happened: https://github.com/mkoeppe/sage/actions/runs/5039255345/jobs/9037285404#step:10:6296

[info-6.8] installing. Log file: /sage/logs/pkgs/info-6.8.log
  [gap-4.12.2] successfully installed.
make --no-print-directory jmol-SAGE_LOCAL-no-deps
[jmol-14.29.52] installing. Log file: /sage/logs/pkgs/jmol-14.29.52.log
  [jmol-14.29.52] successfully installed.
make --no-print-directory libbraiding-SAGE_LOCAL-no-deps
[libbraiding-1.1] installing. Log file: /sage/logs/pkgs/libbraiding-1.1.log
  [libbraiding-1.1] successfully installed.
make --no-print-directory linbox-SAGE_LOCAL-no-deps
[linbox-1.6.3.p1] installing. Log file: /sage/logs/pkgs/linbox-1.6.3.p1.log
  [info-6.8] successfully installed.
...
[maxima-5.46.0] installing. Log file: /sage/logs/pkgs/maxima-5.46.0.log

@mkoeppe
Copy link
Contributor

mkoeppe commented May 22, 2023

Also, for some reason, texinfo is not in the list of the system packages being installed in the GH Actions run in question,
I don't know why.

No, texinfo is installed by Fedora, but our configure complained:

## ----------------------------------------------------- ##
## Checking whether SageMath should install SPKG info... ##
## ----------------------------------------------------- ##
configure:30165: checking for info
configure:30203: result: no
configure:30294: no suitable system package found for SPKG info

Looks like we need to add the separate package info to build/pkgs/info/distros/fedora.txt

@dimpase
Copy link
Member Author

dimpase commented May 22, 2023

Looks like we need to add the separate package info to build/pkgs/info/distros/fedora.txt

good catch. Done

@mkoeppe
Copy link
Contributor

mkoeppe commented May 23, 2023

Thanks. Build is looking good now on all Linux platforms - https://github.com/mkoeppe/sage/actions/runs/5051686843

Copy link
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM re portability.
I haven't followed the discussion regarding the tests.

@github-actions
Copy link

Documentation preview for this PR (built with commit 42a9042) is ready! 🎉

@orlitzky
Copy link
Contributor

I haven't followed the discussion regarding the tests.

They're not a deal-breaker; it's fine by me as well.

A few of the EXAMPLES that fail with maxima-5.46 now have # random tags, but they don't show anything new, and will be bad examples in the future because you can only follow along with an old maxima. Similarly I think we had one TESTS which now has a # random tag. But an invisible test that isn't tested is just wasting cycles.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 24, 2023

Then it sounds like we can set it to positive review. I'll do this now, but if there other concerns, as usual please feel free to revert.

@vbraun vbraun merged commit 4ddf932 into sagemath:develop May 28, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 28, 2023
@mkoeppe
Copy link
Contributor

mkoeppe commented May 29, 2023

So probably we should build maxima with make -j1

@dimpase, we need to do this fix as a follow-up. This build error has showed up again in the 10.1.beta1 test -
https://github.com/sagemath/sage/actions/runs/5103657597/jobs/9174057935#step:13:9737

@dimpase
Copy link
Member Author

dimpase commented May 29, 2023

is it this:
https://sourceforge.net/p/maxima/bugs/3699/
?

a ecl bug/feature?

@mkoeppe
Copy link
Contributor

mkoeppe commented May 29, 2023

I think it's simply that https://sourceforge.net/p/maxima/code/ci/master/tree/doc/info/Makefile.am executes two copies of ./build-html.sh via the rule in line 696. These two copies then can race in creating and removing the directory tmp_html

(The two invocations are in https://github.com/sagemath/sage/actions/runs/5103657597/jobs/9174057935#step:13:9732 and https://github.com/sagemath/sage/actions/runs/5103657597/jobs/9174057935#step:13:9734)

@dimpase
Copy link
Member Author

dimpase commented May 29, 2023

if so, it's easy to fix, just split this into two rules, and make the 2nd of them depend on the 1st, no?

@orlitzky
Copy link
Contributor

...do we need to build the docs? I think they're all still in the release tarball, despite what ./configure implies. The ./configure error stems from,

commit 74168af0d0b3f3be0f2fd0f46f638e45a84a4f42
Author: Robert Dodier <robert_dodier@users.sourceforge.net>
Date:   Sun Nov 21 05:57:35 2021 -0800

    Implement configure option to omit building documentation.

     * configure.ac: recognize new option --enable-build-docs (default = yes)
           and define ENABLE_BUILD_DOCS
     * Makefile.am: execute make in top-level doc directory only if ENABLE_BUILD_DOCS
     * interfaces/xmaxima/Makefile.am: execute make in doc direcory only if ENABLE_BUILD_DOCS

which can be disabled with --disable-build-docs.

@dimpase
Copy link
Member Author

dimpase commented May 29, 2023

I'd rather build the docs, it's quick, and more flexible.

We specifically promoted info spkg to standard, and made it install/check for makeinfo.

@orlitzky
Copy link
Contributor

Ok, I see now that you already had --disable-build-docs in an earlier commit and removed it.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 29, 2023

if so, it's easy to fix, just split this into two rules, and make the 2nd of them depend on the 1st, no?

Sure. Do you want to work on it?

@dimpase
Copy link
Member Author

dimpase commented May 29, 2023

will do

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 6, 2023

I've put a fix on #35652

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 6, 2023

Also, the new dependency on info for ecl and maxima should have been made an order-only dependency.

Not very important, but I'll fix this in #35652, where the incremental workflow is unnecessarily building ecl

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

Successfully merging this pull request may close these issues.

Maxima SPKG: eliminate matrixexp.patch
8 participants