rpms / python3

Clone
Source Code
GIT

#75 Reduced default build flags used to build extension modules
Merged 5 years ago by churchyard. Opened 5 years ago by churchyard.
rpms/ churchyard/python3 extension_cflags  into  master

file modified
+18 -13 
@@ -14,7 +14,7 @@ 
 

  #  WARNING  When rebasing to a new Python version,
 

  #           remember to update the python3-docs package as well
 

  Version: %{pybasever}.2
 

- Release: 5%{?dist}
 

+ Release: 6%{?dist}
 

  License: Python
 

  

  
@@ -167,6 +167,7 @@ 
 

  BuildRequires: openssl-devel
 

  BuildRequires: pkgconfig
 

  BuildRequires: readline-devel
 

+ BuildRequires: redhat-rpm-config >= 127
 

  BuildRequires: sqlite-devel
 

  BuildRequires: gdb
 

  
@@ -436,12 +437,6 @@ 
 

  # See https://fedoraproject.org/wiki/Packaging:Directory_Replacement
 

  Requires: python3-setuptools
 

  

- # https://bugzilla.redhat.com/show_bug.cgi?id=1217376
 

- # https://bugzilla.redhat.com/show_bug.cgi?id=1496757
 

- # https://bugzilla.redhat.com/show_bug.cgi?id=1218294
 

- # TODO change to a specific subpackage once available (#1218294)
 

- Requires: redhat-rpm-config
 

- 

  Provides: %{name}-2to3 = %{version}-%{release}
 

  Provides: 2to3 = %{version}-%{release}
 

  
@@ -549,8 +544,6 @@ 
 

  

  %else  # with flatpackage
 

  

- Requires: redhat-rpm-config
 

- 

  # We'll not provide this, on purpose
 

  # No package in Fedora shall ever depend on flatpackage via this
 

  %global __requires_exclude ^python\\(abi\\) = 3\\..$
 
@@ -650,13 +643,21 @@ 
 

  %endif
 

  

  # Set common compiler/linker flags
 

- export CFLAGS="$RPM_OPT_FLAGS -D_GNU_SOURCE -fPIC -fwrapv"
 

- export CXXFLAGS="$RPM_OPT_FLAGS -D_GNU_SOURCE -fPIC -fwrapv"
 

+ # We utilize the %%extension_...flags macros here so users building C/C++
 

+ # extensions with our python won't get all the compiler/linker flags used
 

+ # in Fedora RPMs.
 

+ # Standard library built here will still use the %%build_...flags,
 

+ # Fedora packages utilizing %%py3_build will use them as well
 

+ # https://fedoraproject.org/wiki/Changes/Python_Extension_Flags
 

+ export CFLAGS="%{extension_cflags} -D_GNU_SOURCE -fPIC -fwrapv"
 

+ export CFLAGS_NODIST="%{build_cflags} -D_GNU_SOURCE -fPIC -fwrapv"
 

+ export CXXFLAGS="%{extension_cxxflags} -D_GNU_SOURCE -fPIC -fwrapv"
 

  export CPPFLAGS="$(pkg-config --cflags-only-I libffi)"
 

- export OPT="$RPM_OPT_FLAGS -D_GNU_SOURCE -fPIC -fwrapv"
 

+ export OPT="%{extension_cflags} -D_GNU_SOURCE -fPIC -fwrapv"
 

  export LINKCC="gcc"
 

  export CFLAGS="$CFLAGS $(pkg-config --cflags openssl)"
 

- export LDFLAGS="$RPM_LD_FLAGS -g $(pkg-config --libs-only-L openssl)"
 

+ export LDFLAGS="%{extension_ldflags} -g $(pkg-config --libs-only-L openssl)"
 

+ export LDFLAGS_NODIST="%{build_ldflags} -g $(pkg-config --libs-only-L openssl)"
 

  

  # We can build several different configurations of Python: regular and debug.
 

  # Define a common function that does one build:
 
@@ -1521,6 +1522,10 @@ 
 

  # ======================================================
 

  

  %changelog
 

+ * Tue Feb 12 2019 Miro Hrončok <mhroncok@redhat.com> - 3.7.2-6
 

+ - Reduced default build flags used to build extension modules
 

+   https://fedoraproject.org/wiki/Changes/Python_Extension_Flags
 

+ 

  * Sat Feb 02 2019 Fedora Release Engineering <releng@fedoraproject.org> - 3.7.2-5
 

  - Rebuilt for https://fedoraproject.org/wiki/Fedora_30_Mass_Rebuild

See https://bugzilla.redhat.com/show_bug.cgi?id=1543394

What needs to be tested:

  • if an extension module is built, does it have the right flags? (i.e. no annobin, specs)
  • if an extension module is built in spec (e.g. with %py3_build), does it have the right flags? (i.e. with annobin, specs)
  • was Python itself built with the proper flags? (i.e. with annobin, specs)

What remains to be done:

  • remove the macro definitions, they shall be in redhat-rpm-config done
  • remove the hard dependency on redhat-rpm-config done
  • do the same with LDFLAG_NODIST https://bugs.python.org/issue35257 done
  • add changelog entry, bump release done

Is this large enough for a self-contained change? If I have to ask, it probably is.

UPDATE: https://fedoraproject.org/wiki/Changes/Python_Extension_Flags

Metadata Update from @churchyard:
- Pull-request tagged with: WIP, blocked, feature
5 years ago

From the CI:

+ python setup.py build_ext --inplace
Compiling module.pyx because it changed.
[1/1] Cythonizing module.pyx
running build_ext
building 'module' extension
creating build
creating build/temp.linux-x86_64-3.7
gcc -pthread -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 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -flto -fuse-linker-plugin -ffat-lto-objects -flto-partition=none -g -fPIC -I/var/str/python/smoke/venv/include -I/usr/include/python3.7m -c module.c -o build/temp.linux-x86_64-3.7/module.o
creating build/lib.linux-x86_64-3.7
gcc -pthread -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -g -flto -fuse-linker-plugin -ffat-lto-objects -flto-partition=none -g build/temp.linux-x86_64-3.7/module.o -L/usr/lib64 -lpython3.7m -o build/lib.linux-x86_64-3.7/module.cpython-37m-x86_64-linux-gnu.so
copying build/lib.linux-x86_64-3.7/module.cpython-37m-x86_64-linux-gnu.so -> 
/var/str/python/smoke/venv/lib64/python3.7/site-packages/Cython/Compiler/Main.py:367: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: /var/str/python/smoke/module.pyx
  tree = Parsing.p_module(s, pxd, full_module_name)

This seem to leak from the LDFLAGS but otherwise works on the CFLAGS part.

Your comment is correct, it indeed leaks from LDFLAGS only now. I'll take a look at the rest the following days.

Trying to use @cstratak's patch to see if it works. Rebased it quite roughly, so it might not even build :D

1 new commit added

  • Add LDFLAGS_NODIST (untested)
5 years ago

rebased onto b979d09e02eb1091b78c05b306af8a7b110eceb2
5 years ago

Rebased, there are no more hacks, but still needs the actual testing.

  • redhat-rpm-config 127 has the necessary flags.
  • Python 3.7.2 should have the patch for LDFLAGS_NODIST. We'll see from the CI log.

The CI link doesn't show up, so here it is.
Edited 5 years ago by churchyard

Metadata Update from @churchyard:
- Pull-request untagged with: blocked
- Pull-request tagged with: review needed
5 years ago

Metadata Update from @churchyard:
- Request assigned
5 years ago

Test of compiling a C extension on the system:

Without the patch:

running build
running build_ext
building 'demo' extension
creating build
creating build/temp.linux-x86_64-3.7
gcc -pthread -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 -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 -flto -fuse-linker-plugin -ffat-lto-objects -flto-partition=none -g -fPIC -I/usr/include/python3.7m -c demo.c -o build/temp.linux-x86_64-3.7/demo.o
creating build/lib.linux-x86_64-3.7
gcc -pthread -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -g -flto -fuse-linker-plugin -ffat-lto-objects -flto-partition=none -g build/temp.linux-x86_64-3.7/demo.o -L/usr/lib64 -lpython3.7m -o build/lib.linux-x86_64-3.7/demo.cpython-37m-x86_64-linux-gnu.so

With the patch:

running build
running build_ext
building 'demo' extension
creating build
creating build/temp.linux-x86_64-3.7
gcc -pthread -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 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -fPIC -I/usr/include/python3.7m -c demo.c -o build/temp.linux-x86_64-3.7/demo.o
creating build/lib.linux-x86_64-3.7
gcc -pthread -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -g build/temp.linux-x86_64-3.7/demo.o -L/usr/lib64 -lpython3.7m -o build/lib.linux-x86_64-3.7/demo.cpython-37m-x86_64-linux-gnu.so

%py3_build before the patch:

+ CFLAGS='-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'
+ LDFLAGS='-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld'
+ /usr/bin/python3 setup.py build '--executable=/usr/bin/python3 -s'
Compiling module.pyx because it changed.
[1/1] Cythonizing module.pyx
running build
running build_ext
building 'module' extension
creating build
creating build/temp.linux-x86_64-3.7
gcc -pthread -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 -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 -fPIC -I/usr/include/python3.7m -c module.c -o build/temp.linux-x86_64-3.7/module.o
creating build/lib.linux-x86_64-3.7
gcc -pthread -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -g -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -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 build/temp.linux-x86_64-3.7/module.o -L/usr/lib64 -lpython3.7m -o build/lib.linux-x86_64-3.7/module.cpython-37m-x86_64-linux-gnu.so
/usr/lib64/python3.7/site-packages/Cython/Compiler/Main.py:367: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: /builddir/build/BUILD/module.pyx
  tree = Parsing.p_module(s, pxd, full_module_name)

And after:

+ CFLAGS='-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'
+ LDFLAGS='-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld'
+ /usr/bin/python3 setup.py build '--executable=/usr/bin/python3 -s'
Compiling module.pyx because it changed.
[1/1] Cythonizing module.pyx
running build
running build_ext
building 'module' extension
creating build
creating build/temp.linux-x86_64-3.7
gcc -pthread -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 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -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 -fPIC -I/usr/include/python3.7m -c module.c -o build/temp.linux-x86_64-3.7/module.o
creating build/lib.linux-x86_64-3.7
gcc -pthread -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -g -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -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 build/temp.linux-x86_64-3.7/module.o -L/usr/lib64 -lpython3.7m -o build/lib.linux-x86_64-3.7/module.cpython-37m-x86_64-linux-gnu.so
/usr/lib64/python3.7/site-packages/Cython/Compiler/Main.py:367: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: /builddir/build/BUILD/module.pyx
  tree = Parsing.p_module(s, pxd, full_module_name)

BTW here is a handy spec:

Name:           reproducer
Version:        0.0
Release:        1%{?dist}
Summary:        ...
License:        MIT
BuildRequires:  gcc
BuildRequires:  python3-devel
BuildRequires:  python3-setuptools
BuildRequires:  python3-Cython

%description
...

%prep

%build
cat > module.pyx << EOF
cdef int add(int a, int b):
    return a + b

def two():
    cdef int a = 1
    cdef int b = 1
    return add(a, b)
EOF

cat > setup.py << EOF
from setuptools import setup
from Cython.Build import cythonize

setup(ext_modules = cythonize('module.pyx'))
EOF

%py3_build

%install
%py3_install


%files
/*

Building python-psycopg2 as an rpm without the patch:

https://cstratak.fedorapeople.org/build-no-patch.log

Building the same package with the patch:

https://cstratak.fedorapeople.org/build-with-patch.log

And a diff generated with meld:
https://cstratak.fedorapeople.org/diff.log

For the record, python-psycopg2 does:

export CFLAGS=${RPM_OPT_FLAGS} LDFLAGS=${RPM_LD_FLAGS}
for python in %{python_runtimes} ; do
  $python setup.py build
done

that seems like LDFLAGS is being respected by distutils and CFLAGS is not?

any ideas where in the code is this handled?

So in your examples Miro I see that on CFLAGS this is not there with the patch, however it comes up later in the flags list:

 -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1

And respectively on the LDFLAGS same thing:

-specs=/usr/lib/rpm/redhat/redhat-hardened-ld
Edited 5 years ago by cstratak

here's the deal: the flags are applied twice.

before patch

gcc -pthread -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 -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 -fPIC -DPSYCOPG_DEFAULT_PYDATETIME=1 -DPSYCOPG_VERSION=2.7.5 (dt dec pq3 ext lo64) -DPG_VERSION_NUM=110000 -DHAVE_LO64=1 -I/usr/include/python3.7m -I. -I/usr/include -I/usr/include/pgsql/server -c psycopg/psycopgmodule.c -o build/temp.linux-x86_64-3.7/psycopg/psycopgmodule.o -Wdeclaration-after-statement

after patch

gcc -pthread -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 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -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 -fPIC -DPSYCOPG_DEFAULT_PYDATETIME=1 -DPSYCOPG_VERSION=2.7.5 (dt dec pq3 ext lo64) -DPG_VERSION_NUM=110000 -DHAVE_LO64=1 -I/usr/include/python3.7m -I. -I/usr/include -I/usr/include/pgsql/server -c psycopg/psycopgmodule.c -o build/temp.linux-x86_64-3.7/psycopg/psycopgmodule.o -Wdeclaration-after-statement

It works but produces different build logs.
Edited 5 years ago by churchyard

Last thing to check:

was Python itself built with the proper flags? (i.e. with annobin, specs)

And I also think this needs more people to review, it's extremely head exploding stuff.

I'll also prep the Fedora Change proposal with all the details.

I believe this is the Simple Koji CI build: https://koji.fedoraproject.org/koji/taskinfo?taskID=31776016

This is indeed the correct build. cc'ing @fweimer if it's possible to get some feedback here on the python build.

Setting to blocked as I work on the change proposal. Don't merge.

https://fedoraproject.org/wiki/Changes/Python_Extension_Flags please contribute your changes. Not sure about the Change name.

Metadata Update from @churchyard:
- Pull-request tagged with: blocked
5 years ago

I've made my changes there.

rebased onto 769702494bbf486191a5e08c62f5db005e6372b3
5 years ago

Change approved.

Metadata Update from @churchyard:
- Pull-request untagged with: blocked
5 years ago

Rebased.

rebased onto 33658d649e87785ef9485d9ad268ede4496e2446
5 years ago

To late to push this before the mass rebuild to see what breaks, but we should do it ASAP after.

rebased onto 7b250c5c1f40939560063c4ea1ec919375cfc032
5 years ago

Fedora CI errored, so trying again, [citest]

Will be making various notes while reviewing this.

The OPT flag export in general seems redundant. I don't think it's used anywhere at this point but will need to verify it. See also: https://bugs.python.org/issue9189

From a SPEC sanity POV these flag exports look fine to me. Will investigate on their propagation effects on user and rpm built C/C++ extenstions.

Same as the comment on line 44

I would change the sentence here with something like "user created C/C++ extensions built with our python, won't use all the compiler/linker flags, used within the Fedora buildroot environment."

Maybe also add a link here to the redhat-rpm-config change? https://src.fedoraproject.org/rpms/redhat-rpm-config/c/e80fa1344a49662fec08d650debf793048c87429?branch=master

Or the relevant part from the readme.

Will need to check the effect of losing those requires on a compiled extension module.

I'll link to the change wiki page, it contains all the relevant information. sounds good?

proposal: "We utilize the %%extension_...flags macros here so users building C/C++ extensions with our python won't get all the compiler/linker flags used in Fedora RPMs."

The proposal sounds fine to me.

There is already a link to the change wiki here, but the redhat rpm config gets into more details. It's more of a nitpick, but I'd consider it good practice if anyone would like to look a bit further down the rabbit hole.

(I'll rebase the comments once the review is over. if you wait for my action, let me know.)

Need to check again if the stdlib if compiled with the hardened flags, the sets of flags when compiling C extensions in the system, the set of flags when compiling rpms, if the %py3_build macro correctly sets the flags for the rpms and also a compilation of a custom extension without redhat-rpm-config installed in the system.

The stdlib is actually compiled with the hardened flags, so the change is fine on that front.

Per some other discussions, there are various weird interactions within the Makefile for the OPT flags, however it is reserved for the interpreter and the stdlib, so I'd use the %{build_cflags} there.

By testing https://koji.fedoraproject.org/koji/taskinfo?taskID=32763503 OPT now includes the hardening flags, by changing it to use the base_cflags.

rebased onto 25099f7ef1a0d8d799154758fb9639e04523b550
5 years ago

rebased.

Ok so apparently my understanding of the OPT flag in this case was wrong. Testing on the previous build where OPT contains the extension flags, compiling a C extension with distutils has the annobin specs omitted. In the case of the latest build where OPT uses the build_cflags the annobin specs get hardcoded into the extension, so this will have to change again, apologies for the oversight.

rebased onto 5da67fb9b6d1d40f06c576538a4b83dfd615c321
5 years ago

rebased onto 7073fa50743f9f3eaded0329a6971cf772b92aed
5 years ago

Rebased, bumped release, added changelog entry, removed "experimental" from the PR title and commit message. Reverted OPT to be extension_cflags.
Edited 5 years ago by churchyard

Metadata Update from @churchyard:
- Pull-request untagged with: WIP
5 years ago

Waiting for the CI build to complete for testing the rest of the usecases.

I can verify that in the latest build, the flags from a demo C extension, compiled through distutils do not contain the annobin and the hardened flags. Also removing the redhat-rpm-config requirement is sane. Before we would get:

gcc: fatal error: cannot read spec file ‘/usr/lib/rpm/redhat/redhat-hardened-cc1’: No such file or directory

Which is not the case now.

All packages invoking python3 setup.py build(_ext) without flags have been adapted (except sagemath, where I'm waiting for the scratch build to finish).

rebased onto 6169e75de91a2b3b47f1894c6b9e34f71391e22b
5 years ago

rebased via the web interface to be on top of current master. no changes.

Also the set of flags is correctly set now when compiling rpm's. Namely the hardened and annobin flags appear only once now, on the second set of flags. That is the expected behaviour as the first set is set by distutils and the second by the environment.

And %py3_build correctly sets the hardened flags for the system, which is not the case for using only 'python3 setup.py build' and this is the expected behavior as well.

Final ack from me. The change is functional, and through my testing things seem to go as expected, we tried to be as thorough as possible and I believe we reached the point where we can confidently merge it. We can still do some nitpicking on the comments, but not really required at this point, I believe everything is clear enough and concise. So please merge at will.

Metadata Update from @churchyard:
- Pull-request untagged with: review needed
- Request assigned
5 years ago

@cstratak \o/

We still have time, so if you want to nitpick some comments, go on.
BTW I have added link to redhat-rpm-config commit in the commit message.

CI failure unrelated.

rebased onto 8927d3f
5 years ago

Pull-Request has been merged by churchyard
5 years ago
Metadata

feature

Flags
simple-koji-ci
success
Build completed for 6169e75d
5 years ago
simple-koji-ci
success
Build completed for 7073fa50
5 years ago
simple-koji-ci
success
Build completed for 5da67fb9
5 years ago
simple-koji-ci
success
Build completed for 25099f7e
5 years ago
simple-koji-ci
success
Build completed for 7b250c5c
5 years ago
Fedora CI
error
Package tests: errored
5 years ago
Fedora CI
success
Package tests: passed
5 years ago
simple-koji-ci
success
Build completed for 33658d64
5 years ago
Fedora CI
success
Package tests: passed
5 years ago
Fedora CI
error
Package tests: errored
5 years ago
Fedora CI
success
Package tests: passed
5 years ago
simple-koji-ci
success
Build completed for b8129671
5 years ago
Fedora CI
success
Package tests: passed
5 years ago
simple-koji-ci
success
Build completed for 3eac7958
5 years ago
Changes Summary 1
+18 -13
file changed
python3.spec
Powered by Pagure 5.13.3
Documentation • File an Issue • About this Instance • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.