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

fedora: Add python3-devel system packages, standardize build/pkgs/python3/distros/*.txt, fix cysignals build on fedora #29473

Closed
mkoeppe opened this issue Apr 6, 2020 · 20 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 6, 2020

Currently, on fedora-31-standard we do not find a system package that passes the python3 spkg-configure test (https://github.com/mkoeppe/sage/runs/563300830).

In this ticket, we add the needed system packages to build/pkgs/python3/distros/fedora.txt.

Also, for uniformity, we move some system packages from build/pkgs/cygwin.txt to build/pkgs/python3/distros/cygwin.txt so that they are only available in a standard build but not a minimal build.

We also add the system package information for python3 for some more distributions in preparation for supporting them.

Using system python3 on fedora leads to a compilation error with cysignals, which we work around.

Upstream: Reported upstream. No feedback yet.

CC: @dimpase @kiwifb

Component: porting

Author: Matthias Koeppe

Branch/Commit: 385e7db

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/29473

@mkoeppe mkoeppe added this to the sage-9.1 milestone Apr 6, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

comment:2

This correctly rejects old python3s for fedora < 29 and too new python3 for fedora-32-standard.

It accepts python3.7 on fedora-29-standard (https://github.com/mkoeppe/sage/runs/570853071)
but then runs into trouble compiling cysignals:

    gcc -Wno-unused-result -Wsign-compare -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -fPIC -DCYTHON_CLINE_IN_TRACEBACK=0 -U_FORTIFY_SOURCE -Isrc/cysignals -Isrc -I/sage/local/include -I/usr/include/python3.7m -c build/src/cysignals/signals.c -o build/temp.linux-x86_64-3.7/build/src/cysignals/signals.o -pthread
    In file included from build/src/cysignals/signals.c:1552:
    build/src/cysignals/implementation.c:27:2: error: #error "cysignals must be compiled without _FORTIFY_SOURCE"
     #error "cysignals must be compiled without _FORTIFY_SOURCE"
      ^~~~~
    error: command 'gcc' failed with exit status 1
    Running setup.py install for cysignals: finished with status 'error'

Same on fedora-30-standard, fedora-31-standard.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

New commits:

689a728fedora: Add python3-devel system packages, standardize build/pkgs/python3/distros/*.txt

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

Commit: 689a728

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title fedora: Add python3-devel system packages, standardize build/pkgs/python3/distros/*.txt fedora: Add python3-devel system packages, standardize build/pkgs/python3/distros/*.txt, fix cython build on fedora Apr 8, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title fedora: Add python3-devel system packages, standardize build/pkgs/python3/distros/*.txt, fix cython build on fedora fedora: Add python3-devel system packages, standardize build/pkgs/python3/distros/*.txt, fix cysignals build on fedora Apr 8, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2020

Changed commit from 689a728 to 385e7db

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

385e7dbbuild/pkgs/cysignals/spkg-install.in: Override -Wp,-D_FORTIFY_SOURCE from fedora python3

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

comment:9

What's happening here is that cysignals tries to make sure that _FORTIFY_SOURCE is undefined by using Extension(undef_macros=...) (https://github.com/sagemath/cysignals/blob/master/setup.py#L46). distutils.ccompiler.CCompiler.gen_preprocess_options generates the compiler flag -U_FORTIFY_SOURCE from that.

However, Fedora uses -Wp,-D_FORTIFY_SOURCE=2, which is passed directly to the C preprocessor and overrides the -U flag.
We override that by passing -Wp,-U_FORTIFY_SOURCE via CPPFLAGS in the spkg-install script.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2020

comment:10

This should probably be reported upstream, but to which?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 9, 2020

Upstream: Reported upstream. No feedback yet.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 9, 2020

comment:11

Reported at sagemath/cysignals#120

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 9, 2020

comment:12

This can be observed to work correctly for fedora-29-standard (https://github.com/mkoeppe/sage/runs/572856776), fedora-30-standard (https://github.com/mkoeppe/sage/runs/572856797), fedora-31-standard (https://github.com/mkoeppe/sage/runs/572856814).

Ready for review

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2020

comment:13

Needs review... let's get this into the next beta please

@dimpase
Copy link
Member

dimpase commented Apr 12, 2020

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Apr 12, 2020

comment:14

lgtm. Regarding upstream, not sure if cysignals has a maintainer now, or Jeroen abandoned it.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 12, 2020

comment:15

Thanks!

@vbraun
Copy link
Member

vbraun commented Apr 16, 2020

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

No branches or pull requests

3 participants