Opened 23 months ago
Closed 20 months ago
#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: |
Description (last modified by )
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 23 months ago by
- Branch set to u/mkoeppe/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_
comment:2 Changed 23 months ago by
- Commit set to 88c4e8c946b6e93381b4ef0b035ff4f21a1f30f5
comment:3 Changed 23 months ago by
- Description modified (diff)
- Priority changed from major to minor
- Status changed from new to needs_review
comment:4 Changed 22 months ago by
- Dependencies set to #30779
comment:5 Changed 22 months ago by
- Commit changed from 88c4e8c946b6e93381b4ef0b035ff4f21a1f30f5 to bb32e80ad26c5a2da590d60033b8fc2e265c2889
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4f56517 | Duplicate src/setup.py
|
6fe4d56 | Merge branch 't/30779/duplicate_src_setup_py' into t/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_
|
698a6ea | src/sage_setup/command/sage_build_cython.py: Remove top-level imports from sage.env, Cython
|
bb32e80 | build/pkgs/sagelib/src/setup.py: Make 'setup.py sdist' work without Cython
|
comment:7 Changed 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:8 Changed 21 months ago by
- Cc dimpase added
- Dependencies #30779 deleted
comment:9 follow-up: ↓ 10 Changed 21 months ago by
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 21 months ago by
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: ↓ 13 Changed 21 months ago by
Do we need to support gcc 4.8 ? The last release of gcc 4.8.* was over 5 years ago.
comment:12 Changed 21 months ago by
- 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 21 months ago by
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 21 months ago by
Thanks for the review! I have opened #30876 for the issue with the unset CC
environment variable.
comment:15 Changed 21 months ago by
- Status changed from positive_review to needs_work
Merge conflict
comment:16 Changed 21 months ago by
- Commit changed from bb32e80ad26c5a2da590d60033b8fc2e265c2889 to 8f04684a7704f0eb7541e69771887d8c8acc3b4d
Branch pushed to git repo; I updated commit sha1. New commits:
f53bc50 | Merge commit 'cc96c6dbae448cd361e798a1f29ec5bf10b0c57b' of git://trac.sagemath.org/sage into t/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_
|
8f04684 | Merge tag '9.3.beta2' into t/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_
|
comment:17 Changed 21 months ago by
- Dependencies set to #30709
- Status changed from needs_work to positive_review
comment:18 Changed 20 months ago by
- 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
Branch pushed to git repo; I updated commit sha1. New commits:
src/setup.py: Make 'setup.py sdist' work without Cython