Opened 6 years ago

Closed 6 years ago

#18842 closed defect (fixed)

Really fix cleaning of Sage library

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.8
Component: cython Keywords:
Cc: fbissey, ncohen Merged in:
Authors: Jeroen Demeyer Reviewers: Steven Trogdon
Report Upstream: N/A Work issues:
Branch: 2b0fbaf (Commits, GitHub, GitLab) Commit: 2b0fbaff82a95a4345b8aa70d4af6965a1a23126
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Since #18494, we install .pxd files but we never remove them.

Change History (15)

comment:1 Changed 6 years ago by ncohen

  • Cc ncohen added

comment:2 Changed 6 years ago by strogdon

Here a 

rm local/lib/python2.7/site-packages/sage/graphs/graph_decompositions/*.pxd

seemed to allow one to recover, but then required a make doc-clean && make to rebuild the docs.

comment:3 Changed 6 years ago by strogdon

Actually, only fast_digraph.pxd and vertex_separation.pxd have to be removed which are new with ticket #18746.

comment:4 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/cythonization_broken

comment:5 Changed 6 years ago by jdemeyer

  • Commit set to 2b0fbaff82a95a4345b8aa70d4af6965a1a23126
  • Summary changed from Cythonization broken to Really fix cleaning of Sage library

New commits:

​2b0fbafConsider all installed files for cleaning up

comment:6 follow-up: Changed 6 years ago by fbissey

So i see you are switching to only using data_files and the only headers to be considered are those found in SAGE_CYTHONIZED which makes sense as all the useful headers will be copied.

Is the issue only happening when one does an upgrade or a re-build where as your commit suggest we may have obsolete .pxd, .pyx or .h files being kept while they should have been deleted?

comment:7 follow-up: Changed 6 years ago by jdemeyer

Hmm... the basic idea of my commit works, but the problem is the order: this cleaning needs to be done before cythonization.

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

Replying to fbissey:

Is the issue only happening when one does an upgrade or a re-build where as your commit suggest we may have obsolete .pxd, .pyx or .h files being kept while they should have been deleted?

Yes, if we delete a .pxd file from the sources, it should also be deleted from the installation directory.

comment:9 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:10 in reply to: ↑ 7 Changed 6 years ago by strogdon

Replying to jdemeyer:

Hmm... the basic idea of my commit works, but the problem is the order: this cleaning needs to be done before cythonization.

Bummer. You're absolutely correct. And this is what is done when they are removed manually.

comment:11 Changed 6 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

comment:12 Changed 6 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Priority changed from blocker to major
  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to N/A
  • Status changed from new to needs_review

See #18851 for the Cython patch.

This branch is still relevant, but it doesn't fix the problem originally reported.

comment:13 Changed 6 years ago by strogdon

  • Reviewers set to Steven Trogdon
  • Status changed from needs_review to positive_review

Built Sage on top of 6.8.beta6 with commits from this ticket and #18851 + #18746. All .pxd files added with #18746 were removed when Sage was rebuild without #18746. François, feel free to reverse things if you see anything out of place with the commits.

comment:14 Changed 6 years ago by fbissey

It's absolutely fine with me and definitely should go in.

comment:15 Changed 6 years ago by vbraun

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