#30580 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: minor Milestone: sage-9.3
Component: build Keywords:
Cc: gh-tobiasdiez, jhpalmieri, fbissey, dimpase Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 8f04684 (Commits, GitHub, GitLab) Commit: 8f04684a7704f0eb7541e69771887d8c8acc3b4d
Dependencies: #30709 Stopgaps:

Status badges

Description (last modified by mkoeppe)

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

Change History (18)

comment:1 Changed 15 months ago by mkoeppe

  • Branch set to u/mkoeppe/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_

comment:2 Changed 15 months ago by git

  • Commit set to 88c4e8c946b6e93381b4ef0b035ff4f21a1f30f5

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

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

comment:3 Changed 15 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Description modified (diff)
  • Priority changed from major to minor
  • Status changed from new to needs_review

comment:4 Changed 14 months ago by mkoeppe

  • Dependencies set to #30779

comment:5 Changed 14 months ago by git

  • Commit changed from 88c4e8c946b6e93381b4ef0b035ff4f21a1f30f5 to bb32e80ad26c5a2da590d60033b8fc2e265c2889

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

comment:6 Changed 14 months ago by mkoeppe

  • Description modified (diff)

Ready for review.

comment:7 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:8 Changed 13 months ago by mkoeppe

  • Cc dimpase added
  • Dependencies #30779 deleted

comment:9 follow-up: Changed 13 months ago by 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
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
...

comment:10 in reply to: ↑ 9 Changed 13 months ago by mkoeppe

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:
# * http://trac.sagemath.org/sage_trac/ticket/14460
# * http://trac.sagemath.org/sage_trac/ticket/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.

comment:11 follow-up: Changed 13 months ago by dimpase

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

comment:12 Changed 13 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm - the gcc version could be bumped elsewhere

comment:13 in reply to: ↑ 11 Changed 13 months ago by mkoeppe

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.

comment:14 Changed 13 months ago by mkoeppe

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

comment:15 Changed 12 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:16 Changed 12 months ago by git

  • Commit changed from bb32e80ad26c5a2da590d60033b8fc2e265c2889 to 8f04684a7704f0eb7541e69771887d8c8acc3b4d

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_

comment:17 Changed 12 months ago by mkoeppe

  • Dependencies set to #30709
  • Status changed from needs_work to positive_review

comment:18 Changed 12 months ago by vbraun

  • Branch changed from u/mkoeppe/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_ to 8f04684a7704f0eb7541e69771887d8c8acc3b4d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.