Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

Setup fails in Python 3 when splitting string #35

Open
tomchor opened this issue Feb 21, 2018 · 5 comments
Open

Setup fails in Python 3 when splitting string #35

tomchor opened this issue Feb 21, 2018 · 5 comments

Comments

@tomchor
Copy link
Contributor

tomchor commented Feb 21, 2018

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
Copy link
Owner

pressel commented Feb 21, 2018

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
Copy link
Contributor Author

tomchor commented Feb 21, 2018

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
Copy link
Owner

pressel commented Feb 21, 2018

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
Copy link
Contributor Author

tomchor commented Feb 21, 2018

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.

#36

@pressel
Copy link
Owner

pressel commented Feb 21, 2018

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

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

No branches or pull requests

2 participants