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

sage_setup: Remove import-time dependency (setup_requires) on pkgconfig, numpy #30580

Closed
mkoeppe opened this issue Sep 16, 2020 · 23 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 16, 2020

Just loading src/setup.py already pulls in Cython, numpy, and pkgconfig via sage_setup, so these distributions would have to be declared as [build_system] requires in src/pyproject.toml (ex setup_requires)

By moving some computations from import-time to runtime, we get rid of this early dependency on pkgconfig, numpy. (They are, of course, still required for building the package.)

This makes setup.py sdist work using a Python that does not have numpy or pkgconfig installed. To test (with a system python that has Cython):

  $ (cd build/pkgs/sagelib/src && python3 -u setup.py --no-user-cfg sdist)

(We also reduce the load-time dependency on Cython; however, we do not address the whole load-time dependency of setup.py on Cython (via sage_setup.find, which uses open_source_file and is_package_dir) in this ticket. This is best done after #28925.)

Depends on #30709

CC: @tobiasdiez @jhpalmieri @kiwifb @dimpase

Component: build

Author: Matthias Koeppe

Branch/Commit: 8f04684

Reviewer: Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Sep 16, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2020

Commit: 88c4e8c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2020

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

88c4e8csrc/setup.py: Make 'setup.py sdist' work without Cython

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2020

Dependencies: #30779

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2020

Changed commit from 88c4e8c to bb32e80

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4f56517Duplicate src/setup.py
6fe4d56Merge branch 't/30779/duplicate_src_setup_py' into t/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_
698a6easrc/sage_setup/command/sage_build_cython.py: Remove top-level imports from sage.env, Cython
bb32e80build/pkgs/sagelib/src/setup.py: Make 'setup.py sdist' work without Cython

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 20, 2020

comment:6

Ready for review.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 22, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 6, 2020

Changed dependencies from #30779 to none

@dimpase
Copy link
Member

dimpase commented Nov 7, 2020

comment:9

the 1st line of the output seems to try to say something about --version I don't get:

$ python3 -u setup.py --no-user-cfg sdist >/tmp/res
/bin/sh: 1: --version: not found
distributions = ['']
warning: sdist: standard file not found: should have one of README, README.txt, README.rst

warning: no files found matching '*.hh' anywhere in distribution
...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 7, 2020

comment:10

Replying to @dimpase:

the 1st line of the output seems to try to say something about --version I don't get:

$ python3 -u setup.py --no-user-cfg sdist >/tmp/res
/bin/sh: 1: --version: not found

This is from src/sage_setup/command/sage_build_cython.py:

# Work around GCC-4.8 bug which miscompiles some sig_on() statements:
# * https://github.com/sagemath/sage-prod/issues/14460
# * https://github.com/sagemath/sage-prod/issues/20226
# * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56982
if subprocess.call("""$CC --version | grep -i 'gcc.* 4[.]8' >/dev/null """, shell=True) == 0:
    extra_compile_args.append('-fno-tree-copyrename')

This gives an (ignored) error when CC is not set -- this defect is not new in this ticket.

@dimpase
Copy link
Member

dimpase commented Nov 8, 2020

comment:11

Do we need to support gcc 4.8 ? The last release of gcc 4.8.* was over 5 years ago.

@dimpase
Copy link
Member

dimpase commented Nov 8, 2020

comment:12

lgtm - the gcc version could be bumped elsewhere

@dimpase
Copy link
Member

dimpase commented Nov 8, 2020

Reviewer: Dima Pasechnik

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 9, 2020

comment:13

Replying to @dimpase:

Do we need to support gcc 4.8 ? The last release of gcc 4.8.* was over 5 years ago.

Yes until we drop ubuntu trusty and similar.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 9, 2020

comment:14

Thanks for the review! I have opened #30876 for the issue with the unset CC environment variable.

@vbraun
Copy link
Member

vbraun commented Nov 29, 2020

comment:15

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2020

Changed commit from bb32e80 to 8f04684

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2020

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

f53bc50Merge commit 'cc96c6dbae448cd361e798a1f29ec5bf10b0c57b' of git://trac.sagemath.org/sage into t/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_
8f04684Merge tag '9.3.beta2' into t/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2020

Dependencies: #30709

@vbraun
Copy link
Member

vbraun commented Dec 6, 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