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

Move CYTHON_CACHE_DIR to a Sage specific directory #25293

Closed
saraedum opened this issue May 4, 2018 · 13 comments
Closed

Move CYTHON_CACHE_DIR to a Sage specific directory #25293

saraedum opened this issue May 4, 2018 · 13 comments

Comments

@saraedum
Copy link
Member

saraedum commented May 4, 2018

Currently the Cython cache (disabled) would be written to ~/.cycache and ~/.cython/inline. Since Sage ships its own (possibly patched) version of Cython, we should not mix our cache with the cache of the Cython that might be installed by the distribution or the user.

CC: @jdemeyer

Component: cython

Author: Julian Rüth

Branch/Commit: u/saraedum/cycache @ 51d813a

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

@saraedum saraedum added this to the sage-8.3 milestone May 4, 2018
@saraedum
Copy link
Member Author

saraedum commented May 4, 2018

Branch: u/saraedum/cycache

@saraedum
Copy link
Member Author

saraedum commented May 4, 2018

comment:2

jdemeyer: I think you know well how sage-env is supposed to work. Do you think this is the right way of doing this?


New commits:

51d813aSetup CYTHON_CACHE_DIR for when we enable Cython caching again

@saraedum
Copy link
Member Author

saraedum commented May 4, 2018

Commit: 51d813a

@jdemeyer
Copy link

jdemeyer commented May 5, 2018

comment:3

I don't agree with this:

the version of Cython that
writes to CYTHON_CACHE_DIR might be incompatible with the (patched) version
shipped with Sage.

If Cython gives the same fingerprint for different Cython versions, that's clearly a bug in Cython.

@saraedum
Copy link
Member Author

saraedum commented May 6, 2018

comment:4

The version is part of the fingerprint but any patches that we add are not part of the fingerprint.

@jdemeyer
Copy link

jdemeyer commented May 7, 2018

comment:5

Replying to @saraedum:

The version is part of the fingerprint but any patches that we add are not part of the fingerprint.

Well, then it's still a bug. Where in the upstream code is this check being done?

@jdemeyer
Copy link

jdemeyer commented May 7, 2018

comment:6

I don't see the need for a Sage-specific cycache directory. We don't do that for ccache either.

@saraedum
Copy link
Member Author

saraedum commented May 7, 2018

comment:7

Replying to @jdemeyer:

Replying to @saraedum:

The version is part of the fingerprint but any patches that we add are not part of the fingerprint.

Well, then it's still a bug. Where in the upstream code is this check being done?

I don't think it's a bug. Upstream makes sure that released versions of cython work properly. If you start to patch them, then you are on your own.

@saraedum
Copy link
Member Author

saraedum commented May 7, 2018

comment:8

Replying to @jdemeyer:

I don't see the need for a Sage-specific cycache directory. We don't do that for ccache either.

I think we do that for ccache (see the line just above the one I am adding here.) But I am very happy not to introduce a sage-specific cycache. (It was mentioned in #17851 that we should do that.)

@saraedum saraedum removed this from the sage-8.3 milestone May 7, 2018
@jdemeyer
Copy link

jdemeyer commented May 7, 2018

comment:9

Replying to @saraedum:

I think we do that for ccache

That's CCACHE_BASEDIR which is specifically meant to share caches. It's basically setting the environment prefix such that ccache can ignore it. So it's rather the opposite of what you propose here.

@jdemeyer
Copy link

jdemeyer commented May 7, 2018

comment:10

Replying to @saraedum:

I don't think it's a bug. Upstream makes sure that released versions of cython work properly. If you start to patch them, then you are on your own.

I'm not following you here... of course we will make sure that the Cython-in-Sage works correctly.

What I'm asking is the following: is it possible for two different Cython installations that produce different C code to accidentally re-use the same cache file?

  • If yes: it's a Cython bug

  • If no: no need for the Sage-specific cycache directory

I don't know exactly how ccache solves this problem, but it seems to work: you can mix GCC versions and ccache knows the difference.

@saraedum
Copy link
Member Author

saraedum commented May 8, 2018

comment:11

I'm not following you here... of course we will make sure that the Cython-in-Sage works correctly.

Say if we backported a patch (without changing Cython's version number - and it is unrealistic to assume that people will remember to change that version number) then it is likely that our Cython and the official Cython behave somewhat differently. If that difference in behaviour is "after" the cache lookup, then you "break" Cython caching in some sense.

But that's an academic discussion. In practice this won't happen.

Is it possible for two released versions of Cython to use the cache incorrectly, ideally not anymore; and sure, if it were then it would be a bug in Cython.

Let's close this issue.

@jdemeyer
Copy link

jdemeyer commented May 9, 2018

comment:12

Replying to @saraedum:

if it were then it would be a bug in Cython.

Yes, that's basically what I have been saying all the time on this ticket.

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

2 participants