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

configure.ac: Remove tests for outdated platforms #36915

Merged
merged 16 commits into from
Feb 13, 2024

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Dec 18, 2023

Likewise for some configuration tests in some spkg-configure.m4 and spkg-install scripts.

Cygwin, AIX, Solaris, ancient OS X, etc.

📝 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

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 5, 2024

What is an outdated platform? If a platform does not run an OS in the list of our release tour, is that outdated?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 5, 2024

Yes. Basically everything except for Linux and macOS.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 5, 2024

... also removed here:

  • Tests/workarounds for ancient compilers such as GCC 4.x
  • 32-bit macOS

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 5, 2024

Otherwise lgtm.

may not work. If you would like to help port Sage to $host,
please join the sage-devel discussion list - see
http://groups.google.com/group/sage-devel
The Sage community would also appreciate any patches you submit]]);;
Copy link
Member

@dimpase dimpase Jan 5, 2024

Choose a reason for hiding this comment

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

I'd have left these lines 214/8 in. It doesn't harm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you understand that it is for example inviting people who try to build it on Cygwin to help us port it to Cygwin?

Copy link
Member

Choose a reason for hiding this comment

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

In 2024, I don't think anyone in their right mind would try porting anything to Cygwin

Copy link
Member Author

Choose a reason for hiding this comment

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

See, that's why I'm removing this line here. It's a milder solution than including the warning "If you dare to even mention your unsupported platform, a certain senior developer will unleash uncontrolled abuse on you."

Copy link
Member

Choose a reason for hiding this comment

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

Are you going to remove all the "you welcome to contribute" everywhere in the docs too?

Copy link
Member

Choose a reason for hiding this comment

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

Your favourite, and supported, platform, macOS, is a pile of merde, pardon my French.

Just by the way.

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 7, 2024

Would you rebase to develop? I think CI does not do that.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 7, 2024

Rebased

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

Thanks.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 8, 2024

Thank you!

@vbraun
Copy link
Member

vbraun commented Jan 12, 2024

I'm getting (but only after a make distclean)

sage -t --long --warn-long 39.6 --random-seed=123 src/sage/rings/complex_double.pyx
**********************************************************************
File "src/sage/rings/complex_double.pyx", line 2290, in sage.rings.complex_double.ComplexDoubleElement.agm
Failed example:
    a.agm(b, algorithm='principal')  # rel tol 1e-15
Expected:
    0.33817546298618006 - 0.013532696956540503*I
Got:
    0.33817546298618006 - 0.013532696956540465*I
Tolerance exceeded in 1 of 2:
    - 0.013532696956540503 vs - 0.013532696956540465, tolerance 3e-15 > 1e-15
**********************************************************************

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 12, 2024

Which system

@vbraun
Copy link
Member

vbraun commented Jan 31, 2024

Fedora 39, kernel 6.6.11-200.fc39.x86_64, Ryzen 7 5800X

Its pretty clear that its just a case of numerical noise, how about upping the tolerance?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 31, 2024

config.log please?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 31, 2024

I can also open a subset PR to bisect this if that's easier

@vbraun
Copy link
Member

vbraun commented Jan 31, 2024

config.log.36915.before.txt
config.log.36915.after.txt

The diff contains

 CDDEXECGMP=''
-CFLAGS='-g -O2'
+CFLAGS=''
 CFLAGS_MARCH='-march=native'

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 31, 2024

OK, thanks. I'll investigate.

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 1, 2024

Apparently caused by 9ea8982

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 1, 2024

Doing this:

diff --git a/configure.ac b/configure.ac
index a24562ea1a7..65937a73cf2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -344,12 +344,6 @@ AX_PROG_PERL_VERSION([5.8.0],[],[
 # Check C/C++/Fortran compilers
 ###############################################################################
 
-# Save compiler flags as configured by the user.
-# We have to redo this, because otherwise `AC_PROG_CC` will just overwrite them.
-AC_SUBST(CFLAGS, "$CFLAGS")
-AC_SUBST(CXXFLAGS, "$CXXFLAGS")
-AC_SUBST(FCFLAGS, "$FCFLAGS")
-AC_SUBST(F77FLAGS, "$F77FLAGS")
 AC_ARG_VAR(CFLAGS, C compiler flags)
 AC_ARG_VAR(CXXFLAGS, C compiler flags)
 AC_ARG_VAR(FCFLAGS, Fortran compiler flags)

repairs the CFLAGS, but this is part of the "march=native" stuff, which I don't want to touch right now.

So I'll just drop commit 9ea8982 here.

Copy link

github-actions bot commented Feb 1, 2024

Documentation preview for this PR (built with commit e0548a0; changes) is ready! 🎉

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 7, 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 -->

Likewise for some configuration tests in some `spkg-configure.m4` and
`spkg-install` scripts.

Cygwin, AIX, Solaris, ancient OS X, etc.

<!-- 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. -->

### 📝 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.
- [ ] 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#36915
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit 3942b62 into sagemath:develop Feb 13, 2024
20 of 21 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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

4 participants