search

igormarfin / shedskin

Automatically exported from code.google.com/p/shedskin
0 stars 0 forks source link

Makefile definitions #124

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
In the generated makefiles, there are multiple references to the location of 
Python headers and shedskin's library code. This causes issues when Shedskin is 
run on one computer and then the output is sent to another computer to be 
compiled. If the other computer has a different version of python as the 
default or installed Shedskin differently (/usr/share/shedskin/ or 
/usr/share/python2.x/...) they have to run a find-and-replace on the 
makefile(s).

Running a find-and-replace isn't the problem so much as the confusing errors 
make outputs either when it can't find, for instance, Python.h or when it can't 
find the sys.cpp target: "No rule to make sys.cpp".

If the generated makefiles instead had these lines at the top, it would signal 
more clearly what to change, and allow single-line edits to affect the whole 
makefile:
PYTHONDIR = /usr/share...
SHEDSKINDIR = /usr/share/shedskin/...

Of course, with the correct directories as found by Shedskin.

Original issue reported on code.google.com by fahh...@gmail.com on 16 Dec 2010 at 12:26

GoogleCodeExporter commented 8 years ago 
agreed, thanks for the suggestion. this has been bothering me for a long time 
as well. let's improve this before 0.8. would you be interesting in writing a 
patch for this?

Original comment by mark.duf...@gmail.com on 16 Dec 2010 at 12:45

GoogleCodeExporter commented 8 years ago 
I'm working on making the patch, but since I'm not as familiar with the code as 
you, I was hoping you could answer a quick question: How can I find the folder 
of the library modules?

I don't want to make any false assumptions so I can't check for paths including 
shedskin/lib nor absolute paths because these seem changeable in the future.

Original comment by fahh...@gmail.com on 17 Dec 2010 at 8:25

GoogleCodeExporter commented 8 years ago 
getgx().sysdir should give you the main shedskin dir, and getgx().libdir the 
/lib dir under that (if you're interested, sysdir is determined as follows in 
shedskin/shared.py: self.sysdir = '/'.join(__file__.split(os.sep)[:-1]). please 
let me know if you have other questions!

Original comment by mark.duf...@gmail.com on 17 Dec 2010 at 8:45

GoogleCodeExporter commented 8 years ago 
Alright, here's 4 patches. I was there, so I thought I might add some other 
small features too.

Original comment by fahh...@gmail.com on 17 Dec 2010 at 10:35

Attachments:

GoogleCodeExporter commented 8 years ago 
great, thanks! :-) I pushed 0001 and 0003. I think 0004 would only be useful 
for debugging, if it were useful at all, which I don't really think it is.. :-) 
I'm not sure about 0002 - I'm hesitant to add more than a few lines of code 
without much benefit.

if you'd be interested in improving things further, the special casing for 're' 
is rather ugly here, and could probably be replaced by a single 'import re' 
kind of thing somewhere else.. it's probably a good idea to split up the 
makefile code into several functions as well. and if you'd also be interested 
in looking at other parts, there's the 'easy tasks' wiki as well.. :-)

thanks again!

Original comment by mark.duf...@gmail.com on 17 Dec 2010 at 12:07

GoogleCodeExporter commented 8 years ago 
So 0002 was just because I've seen many Makefiles with wrapped TARGETS 
variables because they can get pretty long. For Shedskin, if there are a bunch 
of standard libraries imported, the two lines get really, really long. With 
0001 pushed, the lines will be shorter, but still very long.

0004 was just because I re-added it and being able to see that whatever edits 
that were just made caused the templates to increase/decrease significantly is 
a major feature to me. Many times I've introduced a shedskin explosion that I 
only caught before it caused a problem due to the templates number.

I don't know why 're' is special-cased, does your code always need it or just 
in certain cases? Are there functions that are general but related to regexes 
so you put them in there?

I'll see what I can do, class attributes will be useful for something else I'm 
writing so if it comes down to that I'll try to patch it in.

Original comment by fahh...@gmail.com on 17 Dec 2010 at 12:27

GoogleCodeExporter commented 8 years ago 
Btw, this can be marked fixed :)

Original comment by fahh...@gmail.com on 17 Dec 2010 at 2:16

GoogleCodeExporter commented 8 years ago 
regarding 0002, is it really a problem if lines get long in a Makefile..? if 
you insist, perhaps the 'textwrap' module could make this much shorter? :)

about 0004, if you ever encounter a really bad explosion, esp. if it causes an 
abort after the max number of iterations, I'd really like to know about it.. 
:-) I'm thinking about another scalability improvement for 0.8, and I could use 
as many bad examples as possible.. but in any case, I'm wondering what the 
number of templates tells you more than simply timing the shedskin run, as I 
think this is usually very much related? ('time shedskin x.py').

the regular expression module is always needed, because lib/builtin.cpp uses it 
to parse complex numbers.. so it may even be a better idea to avoid this, or 
use PCRE directly..

I will mark the issue as 'fixed' when we agree on a solution for 0002.. :-)

Original comment by mark.duf...@gmail.com on 18 Dec 2010 at 11:52

GoogleCodeExporter commented 8 years ago 
I haven't found a really bad one, but a bunch where I re-used a variable later 
with the same name but different type, or when I convert in-place a variable 
"st = int(st)" so the templates shoots up and I know something's up, especially 
since I'm not in the habit of timing shedskin runs directly. I guess outputting 
"Took %f seconds to shedskin %d SLOC" could be roughly as useful.

Original comment by fahh...@gmail.com on 18 Dec 2010 at 11:56

GoogleCodeExporter commented 8 years ago 
Using textwrap should be fine, I didn't realize that textwrap.wrap() returns a 
list of lines which I could run the final .join() on. I only wrote all that 
code because I couldn't find a solution that let me choose the line endings, 
but I just read the docs wrong.

Original comment by fahh...@gmail.com on 18 Dec 2010 at 11:59

GoogleCodeExporter commented 8 years ago 
I don't think we want to overload users with too much information. perhaps 
instead we could add a debugging or verbose option to shedskin, that gives a 
more detailed overview than just the total number of templates? (for example, 
which functions had or caused the most templates, which class had the most 
'class' templates, which iterations were the most difficult and which functions 
were involved there, and such.. that might also be useful to me while debugging 
shedskin).

of course when you mix types, shedskin will already warn about that (if not, 
it's a bug).

Original comment by mark.duf...@gmail.com on 18 Dec 2010 at 12:21

GoogleCodeExporter commented 8 years ago 
Yeah, a verbose output would be useful.

Here's a text-wrap based version of 0002 (now 0001)

Original comment by fahh...@gmail.com on 18 Dec 2010 at 12:46

Attachments:

GoogleCodeExporter commented 8 years ago 
thanks ;-) too bad we have to give all these options to wrap, and the resulting 
Makefile also looks a bit weird.. would attached patch be acceptable?

Original comment by mark.duf...@gmail.com on 18 Dec 2010 at 2:07

Attachments:

GoogleCodeExporter commented 8 years ago 
I can't directly apply your patch, and I made a small adjustment to keep the 
changes in one place. How do you apply yours? I make/apply with git 
format-patch and git am.

Original comment by fahh...@gmail.com on 18 Dec 2010 at 3:08

Attachments:

GoogleCodeExporter commented 8 years ago 
sorry, I was in a hurry and hadn't used format-patch before. will do that next 
time. you could apply it with 'patch -p1 < one_per_line.path'. 

I committed this one, but moved the 'all:' line down again (I don't really mind 
co-committing small things like this.. ;)), and also used '\t' again, because 
that's used everywhere else as well..

thanks, and come again! ;)

Original comment by mark.duf...@gmail.com on 18 Dec 2010 at 3:54

GoogleCodeExporter commented 8 years ago 
Yeah, no problem. You have to make the commit(s) first, then run "git 
format-patch -#" where # is the number of latest commits to do. There are other 
ways to run it, but that's the most common way for me. I didn't try -p1, that's 
probably what happened.

I didn't notice the moving of all:, though keeping different changes in 
different commits is a useful guideline for me. That way, if I want to undo one 
thing, I don't have to redo other things later.

Original comment by fahh...@gmail.com on 18 Dec 2010 at 4:20