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

Update the gcc spkg to version 14.2.0 using iains/gcc-14-branch #38905

Merged
merged 5 commits into from
Nov 16, 2024

Conversation

culler
Copy link
Contributor

@culler culler commented Nov 1, 2024

This PR updates the gcc spkg (and hence the gfortran spkg) to version 14.2.0. I tested that the spkg builds cleanly on macOS 15.1 (Sequoia) using XCode 16.1 with both Intel and M1 CPUs. I also tested that it builds cleanly on Ubuntu 22.04 using the system gmp and gcc. The openblas spkg also builds on all three of those systems, although I could not determine whether Sage was using the system gfortran or its own gfortran for building openblas on Ubuntu.

There is a caveat. The gcc project does not currently have official support for Apple Silicon. However, the gcc maintainer for darwin, Iain Sandoe , maintains a github site containing gcc source code with "experimental" support for Apple silicon, in addition to Intel. This PR uses the gcc-14-branch repository from that site. The spkg fetches the tag gcc-14.2-darwin-r2 as a tarball and names the tarball as gfortran-14.2.0.tar.gz. The github api does not support publishing hashes of tag downloads. So I downloaded the tag tarball twice and verified that the files were the same for both downloads. I used hashes of that file in checksums.ini.

No patches are needed for building on macOS with either CPU or on Ubuntu. (cf PR #38855)

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Nov 1, 2024

Documentation preview for this PR (built with commit edd5553; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@jhpalmieri
Copy link
Member

I have an Apple silicon machine with a lot of homebrew packages installed. With ./configure --with-system-gfortran=no, this seems to build fine. With ./configure --with-system-gcc=no, the build fails when it reaches ppl and givaro. This is the same as with the develop branch, so it is not a regression, at least on this machine.

@culler
Copy link
Contributor Author

culler commented Nov 4, 2024

I have never tried building Sage with ./configure --with-system-gcc=no. I have always used the system gcc, which is really clang, but let Sage use its own gfortran to build openblas since there is no system fortran. So this may be a configuration which has not been tested for a long time.

What sort of error messages are you getting? Are you seeing this?

I wonder if givaro and ppl publish requirements for the gcc version.

@jhpalmieri
Copy link
Member

I'm not sure if Sage has ever built on OS X (in recent memory) with --use-system-gcc=no. The error messsages: for ppl

[spkg-install] Undefined symbols for architecture arm64:
[spkg-install]   "operator<<(std::ostream&, __mpq_struct const*)", referenced from:
[spkg-install]       (anonymous namespace)::write_polyhedron(std::ostream&, Parma_Polyhedra_Library::C_Polyhedron const&, (anonymous namespace)::Representation) in ppl_lcdd.o
[spkg-install]   "operator<<(std::ostream&, __mpz_struct const*)", referenced from:
[spkg-install]       void (anonymous namespace)::guarded_write<__gmp_expr<__mpz_struct [1], __mpz_struct [1]>>(std::ostream&, __gmp_expr<__mpz_struct [1], __mpz_struct [1]> const&) in ppl_lcdd.o
[spkg-install]   "operator>>(std::istream&, __mpq_struct*)", referenced from:
[spkg-install]       (anonymous namespace)::read_coefficients(std::istream&, (anonymous namespace)::Number_Type, std::vector<__gmp_expr<__mpz_struct [1], __mpz_struct [1]>, std::allocator<__gmp_expr<__mpz_struct [1], __mpz_struct [1]>>>&, __gmp_expr<__mpz_struct [1], __mpz_struct [1]>&) in ppl_lcdd.o
[spkg-install]   "operator>>(std::istream&, __mpz_struct*)", referenced from:
[spkg-install]       (anonymous namespace)::read_coefficients(std::istream&, (anonymous namespace)::Number_Type, std::vector<__gmp_expr<__mpz_struct [1], __mpz_struct [1]>, std::allocator<__gmp_expr<__mpz_struct [1], __mpz_struct [1]>>>&, __gmp_expr<__mpz_struct [1], __mpz_struct [1]>&) in ppl_lcdd.o
[spkg-install] ld: symbol(s) not found for architecture arm64

and for givaro

[spkg-install] Undefined symbols for architecture arm64:
[spkg-install]   "operator<<(std::ostream&, __mpz_struct const*)", referenced from:
[spkg-install]       Givaro::Integer::print(std::ostream&) const in libgivaro.9.dylib-master.o
[spkg-install]       Givaro::operator<<(std::ostream&, Givaro::Integer const&) in libgivaro.9.dylib-master.o
[spkg-install]   "operator>>(std::istream&, __mpz_struct*)", referenced from:
[spkg-install]       Givaro::operator>>(std::istream&, Givaro::Integer&) in libgivaro.9.dylib-master.o
[spkg-install] ld: symbol(s) not found for architecture arm64

@culler
Copy link
Contributor Author

culler commented Nov 4, 2024

That seems to be the same error in both cases and it seems to arise in C++ print statements.

I am trying a build with those options on Intel. It takes a really long time to build gcc, which has to happen before almost anything else. It has to build the compiler 3 times and verify that building the third compiler with the second compiler produces an identical copy of the second compiler. Then it starts all over and builds gmp, mpfr and mpc with the new gcc. It is hard to imagine why anyone would ever want to do that as part of building sage. Building the gfortran spkg is trivial by comparison.

It wouldn't surprise me to learn that the previous update to the gfortran package was not tested in this way, even though it modified the gcc spkg, but who knows?

@culler
Copy link
Contributor Author

culler commented Nov 4, 2024

Well, on Intel I got two errors. The first is probably a genuine bug in this version of the gfortran spkg.

EDIT: This is not a bug in the gfortran package, but a "feature" of Sage's configure script. One is expected to know to disable the gfortran package when the gcc package is enabled. This does not happen automatically.

The error message says:

[spkg-preinst] ***************************************************************
[spkg-preinst] Error: The gcc SPKG is already installed and provides gfortran
[spkg-preinst] ***************************************************************
Error running the preinst script for gfortran-14.2.0.

The second error occurs in the ecm spkg. Apparently some of the assembly language files in that package are not compatible with gcc 14 for Intel. The error is:

[spkg-install] /bin/bash ../libtool    --mode=compile gcc  -march=native -g -O2 
-fPIC -c -o mulredc1.lo mulredc1.s
[spkg-install] libtool: compile:  gcc -march=native -g -O2 -fPIC -c mulredc1.s -
o mulredc1.o
[spkg-install] mulredc1.s:63:19: error: unexpected token in '.section' directive
[spkg-install] .section .note.GNU-stack,"",%progbits
[spkg-install]                   ^
[spkg-install] make[7]: *** [mulredc1.lo] Error 1

The file mulredc1.s is created by

m4 -I../ -DOPERATION_mulredc1 `test -f mulredc1.asm || echo './'`mulredc1.asm >mulredc1.s

@jhpalmieri
Copy link
Member

Probably the right solution (not for this PR) is to disable, or at least provide strong warnings against, the option --with-system-gcc=no on OS X.

@culler
Copy link
Contributor Author

culler commented Nov 4, 2024

That sounds reasonable, but I don't know how to do it.

Regarding the ecm error, the problematic lines in the file mulredc1.s look like this:
62 #if defined(linux) && defined(ELF)
63 .section .note.GNU-stack,"",%progbits
64 #endif
So that suggests that the gcc compiler build by the gcc spkg defines both __linux__ and __ELF__ on macOS, even though macOS is not linux and it is presumably building mach-O executables, not ELF. (Or maybe it is checking the .note line even though that block is excluded.). Seems pretty strange.

@culler
Copy link
Contributor Author

culler commented Nov 4, 2024

I found the explanation for the ecm issue here.

The .s extension used by ecm means that the preprocessor should not be run, and hence that the if defined() lines will be ignored. The extension should be .S or .sx in order to run the preprocessor. Changing the name of the file to mulredc1.S eliminates the error. Of course this is made more complicated by the fact that macOS has a case-insensitive filesystem.

Looking at build/ecm-7.0.6/src/x86_64/Makefile.in reveals that this is an upstream bug. The Makefile.in says:

# Nothing here needs the C preprocessor, and including this rule causes
# "make" to build .S, then .s files which fails on case-insensitive 
# filesystems
#.asm.S:
#       $(M4) -I../ -DOPERATION_$* `test -f $< || echo '$(srcdir)/'`$< >$*.S

The claim that "Nothing here needs the C preprocessor" is patently false, since mulredc1.asm and most of the other assembler files contain if defined() directives.

I was able to come up with a patch to fix this. It replaces .s by .sx throughout the Makefile.in.

So I am now continuing my Intel build (with --disable-gfortran added as a configuration option).

edit: givaro built without issues on Intel
edit: ppl also built without issues on Intel

@culler
Copy link
Contributor Author

culler commented Nov 5, 2024

For the record, I also ran into issues with Flint when building with Sage's gcc on my arm system. I think it is true that the arm version of gcc built by this PR is not ready for prime time, but neither was the one which is replaces. There is a reason that the gcc project does not officially support Apple silicon yet. And there seems to be no problem with the gfortran, which is the main focus of github/iains

@jhpalmieri
Copy link
Member

The various linux "minimal" buildbots are failing to build gfortran. See for example https://github.com/sagemath/sage/actions/runs/11672580205/job/32501588787. (I think that "minimal" means that they use very few system packages, so these are the ones that are trying to build Sage's gfortran.)

@culler
Copy link
Contributor Author

culler commented Nov 9, 2024

It looks to me like the failure is caused by not having flex installed. Is there a way to check which packages are installed in those containers?

@jhpalmieri
Copy link
Member

It looks to me like the failure is caused by not have flex installed. Is there a way to check which packages are installed in those containers?

I am very far from an expert in this, but maybe it's the file SAGE_ROOT/tox.ini.

@culler
Copy link
Contributor Author

culler commented Nov 9, 2024

The tox.ini file contains this line:
ubuntu-toolchain-trusty: EXTRA_SYSTEM_PACKAGES=binutils-2.26
I think that is the only package installed for the minimal configuration. But flex is not included in the binutils package.

The flex requirement is documented here: https://gcc.gnu.org/install/prerequisites.html. Looking at the gcc build logs, it appears that some attempt is made to deal with flex being missing but it does not succeed in this case.

@culler
Copy link
Contributor Author

culler commented Nov 9, 2024

Well, I have no idea how to make sure that flex is installed in the "minimal" docker images, nor what that has to do with tox, nor why the current spkg builds without flex (given that gcc says it is required). I can say that this gfortran spkg builds on my ubuntu-24.04 system, which does have flex, and appears to work -- i.e. the openblas spkg builds.

@culler
Copy link
Contributor Author

culler commented Nov 9, 2024

I think I may have figured out how to get flex installed in the docker images: by editing files in build/_prereq/distros. I tried adding it in debian.txt. We will see what the CI runner does.

@culler
Copy link
Contributor Author

culler commented Nov 10, 2024

The various linux "minimal" buildbots are failing to build gfortran.

@jhpalmieri: While some of those CI jobs are still failing, the failures are not happening in the gfortran-14.2 build, as far as I can tell. Some of the failed jobs are using the system gfortran 13, but the ones which do build gfortran 14 seem to do so without errors. So I think that adding flex as a prerequisite fixed this issue.

@jhpalmieri
Copy link
Member

Okay, let's merge it!

@culler
Copy link
Contributor Author

culler commented Nov 15, 2024

Thanks, John!

@vbraun vbraun merged commit 1b8ac9c into sagemath:develop Nov 16, 2024
32 of 49 checks passed
@dimpase
Copy link
Member

dimpase commented Nov 26, 2024

The ecm patch seems to be causing trouble for some X86_64 macOS machines, see

https://groups.google.com/d/msgid/sage-release/E76C1EBD-35A1-4B22-9886-4138307EDAD6%40gmail.com?utm_medium=email

@dimpase
Copy link
Member

dimpase commented Nov 26, 2024

Probably the right solution (not for this PR) is to disable, or at least provide strong warnings against, the option --with-system-gcc=no on OS X.

I think we should just get rid of gcc package (perhaps, leave gfortran). As proposed in #32532

@dimpase
Copy link
Member

dimpase commented Nov 26, 2024

Probably the right solution (not for this PR) is to disable, or at least provide strong warnings against, the option --with-system-gcc=no on OS X.

I don't see why gcc is allowed to be a standard package, as it's pretty much impossible to build Sage on macOS using it, and standard packages are supposed to be working on all supported platforms.

@dimpase
Copy link
Member

dimpase commented Nov 27, 2024

@jhpalmieri - did you test the branch on an Intel mac?

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 28, 2024
The ECM patch added in  sagemath#38905 breaks some macOS installations.

As it was added in order to support building ECM with gcc 14, which is a
rather hypothetical situation, it's OK to drop it.

This will fix sagemath#39039

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

URL: sagemath#39040
Reported by: Dima Pasechnik
Reviewer(s): Kwankyu Lee
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.

4 participants