search

sagemath / sage-patchbot

Sage Patchbot
https://www.sagemath.org
Other
8 stars 17 forks source link

Fixes for unsafe tickets #108

Open kwankyu opened 7 years ago

kwankyu commented 7 years ago

This is related with the issue #98

kwankyu commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

kwankyu commented 7 years ago

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 commented 7 years ago

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).