Opened 3 years ago

Closed 3 years ago

#24471 closed defect (fixed)

Don't put non-built Cython extensions in sage_build_cython.extensions

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.2
Component: build Keywords:
Cc: embray Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: b65d809 (Commits, GitHub, GitLab) Commit: b65d809f286e9aeb3de2213687204128f07ca99d
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The OptionalExtension mechanism (see src/sage_setup/optional_extension.py) allows for Cython modules to be cythonized but not compiled by distutils (by some logic in the sage_build_ext build command).

This can cause problems in the following scenario:

  1. Install some optional package
  1. Build Cython module depending on that optional package
  1. Try to re-install that optional package (for example, because of an upgrade) but the installation fails for some reason.
  1. Build the Sage library. This will just ignore the module from step 2: the old (outdated) copy will be kept.
  1. Run Sage. If the module from step 2 can still be imported, assorted breakage can occur.

This ticket aims to fix step 4: the outdated built module should be removed by the cleaning system clean_install_dir(). This is easy to accomplish by removing those modules from ext_modules.

Change History (8)

comment:1 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/don_t_put_non_built_cython_extensions_in_sage_build_cython_extensions

comment:2 Changed 3 years ago by jdemeyer

  • Commit set to b65d809f286e9aeb3de2213687204128f07ca99d
  • Status changed from new to needs_review

New commits:

b65d809Remove skip_build extensions from ext_modules

comment:3 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:5 follow-up: Changed 3 years ago by embray

Makes sense to me. It might be nice to have a test for this but that would involve adding more tests for sage_setup in general--something I know we've been meaning to do for a while.

comment:6 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

Replying to embray:

It might be nice to have a test for this but that would involve adding more tests for sage_setup in general

The problem is that it's not really possible to test sage_setup without setup.py. So that would require that most logic of setup.py would be moved to the sage_setup module such that it can be imported for testing. I agree that this makes sense, but not for this ticket.

comment:7 Changed 3 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

Yep.

comment:8 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/don_t_put_non_built_cython_extensions_in_sage_build_cython_extensions to b65d809f286e9aeb3de2213687204128f07ca99d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.