Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Really fix cleaning of Sage library #18842

Closed
jdemeyer opened this issue Jul 2, 2015 · 20 comments
Closed

Really fix cleaning of Sage library #18842

jdemeyer opened this issue Jul 2, 2015 · 20 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Jul 2, 2015

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

CC: @kiwifb @nathanncohen

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 2b0fbaf

Reviewer: Steven Trogdon

Issue created by migration from https://trac.sagemath.org/ticket/18842

@jdemeyer jdemeyer added this to the sage-6.8 milestone Jul 2, 2015
@strogdon
Copy link

strogdon commented Jul 2, 2015

comment:2

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.

@strogdon
Copy link

strogdon commented Jul 2, 2015

comment:3

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

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

Branch: u/jdemeyer/cythonization_broken

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

New commits:

2b0fbafConsider all installed files for cleaning up

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

Commit: 2b0fbaf

@jdemeyer jdemeyer changed the title Cythonization broken Really fix cleaning of Sage library Jul 3, 2015
@kiwifb
Copy link
Member

kiwifb commented Jul 3, 2015

comment:6

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?

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

comment:7

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

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

comment:8

Replying to @kiwifb:

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.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

Upstream: Reported upstream. No feedback yet.

@strogdon
Copy link

strogdon commented Jul 3, 2015

comment:10

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.

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

Changed upstream from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

Changed upstream from Reported upstream. Developers acknowledge bug. to none

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

Author: Jeroen Demeyer

@jdemeyer
Copy link
Author

jdemeyer commented Jul 3, 2015

comment:12

See #18851 for the Cython patch.

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

@strogdon
Copy link

strogdon commented Jul 4, 2015

Reviewer: Steven Trogdon

@strogdon
Copy link

strogdon commented Jul 4, 2015

comment:13

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.

@kiwifb
Copy link
Member

kiwifb commented Jul 4, 2015

comment:14

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

@vbraun
Copy link
Member

vbraun commented Jul 5, 2015

Changed branch from u/jdemeyer/cythonization_broken to 2b0fbaf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants