Opened 2 years ago

Closed 2 years ago

#29855 closed defect (fixed)

sagelib setup.py: Fix dependencies on header files of packages gmp, ntl

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: build Keywords:
Cc: mjo, gh-kliem, fbissey, dimpase, jhpalmieri Merged in:
Authors: Matthias Koeppe Reviewers: François Bissey, Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: 18aae7f (Commits, GitHub, GitLab) Commit: 18aae7f8874d6e05d925ebec99ccbe4585f18683
Dependencies: #29702 Stopgaps:

Status badges

Description

These header file locations (for use in Cython dependencies only) are currently hardcoded as living in SAGE_INC, which is no longer true.

This is a step toward #29711, and also preparation for #29847 because it removes use of sage.env.SAGE_INC.

Change History (21)

comment:1 Changed 2 years ago by mkoeppe

  • Branch set to u/mkoeppe/sagelib_setup_py__fix_dependencies_on_header_files_of_packages_gmp__ntl

comment:2 Changed 2 years ago by mkoeppe

  • Cc mjo gh-kliem fbissey added
  • Commit set to 041c9e808a4f28253c232762b3efa37ea306e3c4
  • Status changed from new to needs_review

Last 10 new commits:

​5db5318Trac #29345: remove "break" statements from AC_SEARCH_LIBS.
​e810ad1Trac #29345: don't use sage's config.status for the lrcalc build.
​93c9921Trac #29345: replace the function that populates the CVXOPT_* variables.
​0e66a0aTrac #29345: add Dima's SPKG patches for ksh compatibility.
​df3f05ebuild/make/Makefile.in [SCRIPT_PACKAGE_templ]: cd into the SPKG directory; adjust spkg-install scripts
​5372065Merge branch 't/29793/script_packages_should_cd_into_the_spkg_directory' into t/29411/make_sagelib_a_script_package
​c166b97Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup
​cc30471build/bin/write-dockerfile.sh: Do not ADD removed file src/Makefile.in
​8a41326Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup
​041c9e8sage_setup.command.sage_build_cython: Use SAGE_GMP_PREFIX, SAGE_NTL_PREFIX for header dependencies instead of SAGE_INC

comment:3 Changed 2 years ago by mkoeppe

Branch is on top of #29702; there is only one new commit.

comment:4 Changed 2 years ago by mkoeppe

  • Cc dimpase added

comment:5 follow-up: Changed 2 years ago by mkoeppe

Needs review

comment:6 in reply to: ↑ 5 Changed 2 years ago by fbissey

Replying to mkoeppe:

Needs review

This ticket is only the last commit? Or should I look at more commits?

comment:7 Changed 2 years ago by mkoeppe

That's right, only the last commit.

comment:8 Changed 2 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Hum, you even said so in an earlier. Must be post lunch fatigue :)

LGTM this is much cleaner and much more appropriate.

I really have the feeling that you are breaking sage apart to rebuild it. And I am not complaining.

comment:9 Changed 2 years ago by mkoeppe

Thanks!

comment:10 Changed 2 years ago by mkoeppe

  • Status changed from positive_review to needs_work

comment:11 Changed 2 years ago by mkoeppe

Discovered a stupid mistake in the last commit.

comment:12 Changed 2 years ago by git

  • Commit changed from 041c9e808a4f28253c232762b3efa37ea306e3c4 to 18aae7f8874d6e05d925ebec99ccbe4585f18683

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

​18aae7fsage_setup.command.sage_build_cython: Fix up - add list brackets

comment:13 Changed 2 years ago by fbissey

I hadn't spotted that! So, they need to be dictionaries?

comment:14 Changed 2 years ago by mkoeppe

the values of that dictionary must be lists.

comment:15 Changed 2 years ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:16 Changed 2 years ago by mkoeppe

Ready for another round of review

comment:17 Changed 2 years ago by fbissey

I feel especially stupid to have missed the syntax the first time so I think it should be fair for someone else to check.

comment:18 Changed 2 years ago by mkoeppe

  • Cc jhpalmieri added
  • Reviewers changed from François Bissey to François Bissey, ...

comment:19 Changed 2 years ago by gh-kliem

  • Reviewers changed from François Bissey, ... to François Bissey, Jonathan Kliem
  • Status changed from needs_review to positive_review

LGTM.

comment:20 Changed 2 years ago by mkoeppe

Thank you!

comment:21 Changed 2 years ago by vbraun

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