pressel / pycles

A python based infrastructure for cloud large eddy simulation.
GNU General Public License v3.0
92 stars 69 forks source link

Setup fails in Python 3 when splitting string #35

Open tomchor opened 6 years ago

tomchor commented 6 years ago

Using Python 3, Setup fails for me with this error:

Traceback (most recent call last):
  File "setup.py", line 51, in <module>
    library_dirs = string.split(os.environ['LD_LIBRARY_PATH'],':')
AttributeError: module 'string' has no attribute 'split'

Which I think is because the string.split is a method that exists for Python 2, but doesn't exist for Python 3. This could easily be re-written as

library_dirs = os.environ['LD_LIBRARY_PATH'].split(':')

But I'm guessing there's a reason why it wasn't written like this is the first place, otherwise I would have done a pull request.

pressel commented 6 years ago

You are correct that using sting.split is problematic for Python 3, and there is no good reason why we use it. If you would like to submit a pull request with your suggested fix, I'll happily accept it.

Thanks for letting us know about this issue.

tomchor commented 6 years ago

Fixed it but the compilation still fails with KeyError: 'LD_LIBRARY_PATH', since the path in my Linux Mint machine is $PATH. So the line should read:

library_dirs = os.environ['PATH'].split(':')

Indeed if I change this line it compiles (apparently) fine. (I haven't had time to check the compilation and run examples.)

This brings me to my point: I think your if-else sequence for compilation flags is not taking into account random user computes. For example, if the machine tests True for this statement:

elif platform.machine()  == 'x86_64':
    #Compile flags for fram @ Caltech

you set up the flags as if it were the Caltech FRAM machine. However, there are many other machines that will test positive here and need different flags (mine included). I hope you see my point, but I can elaborate more if you want.

My suggestion is changing the else statement in line 64 (which now gives an error) for a default compilation flag assuming a default (possibly Ubuntu) Linux machine and maybe including a remark about this in the installation instructions.

If you're ok with this, I'd be glad to make a pull request.

pressel commented 6 years ago

That seems reasonable to me. Ideally, what we should do is use something like system.hostname() or socket.gethostname() to determine which machine we are compiling on and provide the correct custom compilation flags, and then as you say also provide compilation flags for a default Linux machine. To be honest, I'm not sure why we aren't doing that already.

I'd appreciate your submitting a pull request for this.

tomchor commented 6 years ago

I think your comment about using hostname() is right. But I didn't want to make alter this in that way because that requires more knowledge about other flags and basically should probably be done by people in your group. I did, however, make a pull request with a simple alteration that will already enable it to be compiled on many more machines right out of the box.

https://github.com/pressel/pycles/pull/36

pressel commented 6 years ago

Thanks for your contribution. For now, I'll leave this issue open as a reminder to switch to using hostname() for Fram builds.