Add support for spaces in paths #4773
Conversation
Make theano support programs and packages installed under paths which include spaces (such as 'Program Files')
| if config.dnn.include_path: | ||
| params.append("-I" + config.dnn.include_path) | ||
| params.extend(['-I"%s"' % config.dnn.include_path]) |
There was a problem hiding this comment.
If you want to also support the GPU, then you should also check the file nvcc_compiler.py in that directory. There are some -L and others options that you didn't touched.
There was a problem hiding this comment.
OK. Thanks for pointing this out.
On my machine I did not have to fix this to make GPU work, so I was unaware of this file.
Make theano support programs and packages installed under paths which include spaces (such as 'Program Files')
Removed extra blank line Removed trailing whitespaces Added whitespaces after ',' Added whitespaces after '=='
| flags += ['-l%s' % l for l in ["mkl_core", | ||
| "mkl_intel_thread", | ||
| "mkl_rt"]] | ||
| res = try_blas_flag(flags) | ||
| if res: | ||
| return res | ||
|
|
||
| # to support path that includes spaces, we need to wrap it with double quotes on Windows | ||
| path_wrapper = "\"" if os.name == 'nt' else "" |
There was a problem hiding this comment.
Is there a reason to do that only on nt? It would make the code easier to understand.
Also, we use sys.platform == 'win32' everywhere else. So if there isn't a reason not to use that instead of os.name, I would prefer that.
Otherwise, the PR look good. thanks.
There was a problem hiding this comment.
Honestly, I am not a huge specialist in platform related issues. The reasons I choose to use os.name == 'nt'condition were:
- This is the condition used in file theano/misc/windows.py (line 13)
- This is the fist solutions that pops-up with google search "python check if windows" ->
go to http://stackoverflow.com/questions/1325581/how-do-i-check-if-im-running-on-windows-in-python (first link in search) and then the most voted solution (105 votes) suggests using os.name, and in the next solution (with sys.platform) you can find 'But os.name is probably the way to go, as others have mentioned.'
I googled the difference between both, and as far as I understand the os.name is more general than the sys.platform. So, os.name could be used to check if I run on any Windows OS, and sys.platform could be used to check if I run on some specific Windows platform. But, once again, I am not a specialist in OS.
I believe the paths should be treated the same across all Windows platforms (alike subprocess is treated the same in windows.py), so os.name is more appropriate here. sys.platform, may be more appropriate for compilation related checks, though.
There was a problem hiding this comment.
thanks for looking into that. It seem I was wrong when I taught we used the same way everywhere. What you tell is pretty convincing:)
|
I was going to merge, but this branch now have conflict. Can you rebase it? thanks |
|
ok to merge when travis pass. |
|
thanks |
Make theano support programs and packages installed on Windows under paths which
include spaces (such as 'Program Files')