Skip to content
This repository has been archived by the owner on Jan 31, 2023. It is now read-only.

Fixes for unsafe tickets #108

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixes for unsafe tickets #108

wants to merge 4 commits into from

Conversation

kwankyu
Copy link

@kwankyu kwankyu commented Jun 26, 2017

This is related with the issue #98

@kwankyu
Copy link
Author

kwankyu commented Jun 26, 2017

The temporary dir is supposed to be deleted in line 1213-1215 patchbot.py, and hence should not be deleted in line 366-369 in trac.py. This bug causes apply failure for unsafe tickets!

@kwankyu
Copy link
Author

kwankyu commented Jun 26, 2017

It is unreasonable to do ./sage -i ccache just after cloning sage to a temporary dir and before first build. I observed that this command just fails, for unsafe tickets. Best to remove it.

@embray
Copy link
Contributor

embray commented Jun 29, 2017

I'm not sure I understand--are you proposing removing the use_ccache feature entirely? And if so, what exactly is that fixing?

@kwankyu
Copy link
Author

kwankyu commented Jun 29, 2017

As I understand it (just by reading the code), ccache is installed by lines 1504-1508 in patchbot.py for normal tickets.

For unsafe tickets, ccache is installed by lines 357-360 in trac.py. But then this code actually fails as it tries to run "./sage" in the temporary folder just created and into which the repo is cloned and importantly before sage is even built. So this is not a right place to run "./sage -i ccache"!

My question is: if you build sage from scratch in a temporary folder for unsafe tickets, then does installing ccache help speed up compiling? If yes, then we need to have ccache installed for unsafe tickets somewhere else (than in "pull_from_trac").

I may be totally misunderstanding the code. If so, let me know.

@embray
Copy link
Contributor

embray commented Jun 29, 2017

I think I see now; trying it myself in a fresh Sage clone, it seems, it will try to install ccache even if sage isn't built yet (just like in the unsafe_ticket case).

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

Successfully merging this pull request may close these issues.

2 participants