search

manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

Fixed compile failure for missing 'CC' key #226

Closed manodeep closed 3 years ago

manodeep commented 3 years ago

Fix for #225

manodeep commented 3 years ago

@lgarrison While fixing this, I noticed that the offending line os.environ['CC'] means that the behaviour is different between command-line installation vs installing through python setup.py. In the command-line version (i.e., make) the environment variable CC is not honoured, whereas in the setup.py the CC variable (if defined) is written out into the common.mk file. At the very least, the two installations (i.e., make and python setup.py install ..) should use the same compiler.

Should we honour the environment variable in common.mk (i.e., change CC := to CC ?=) or remove the os.environ.get('CC', None) line from setup.py?

lgarrison commented 3 years ago

Hmm... maybe we're better off ignoring CC in the environment, as it might be ancient, as you've said before. On the flip side, I just checked all the systems I regularly use, and none of them have CC defined. So maybe we shouldn't be worried about this and respect the environment variable?

manodeep commented 3 years ago

May be modern OS's don't define CC and therefore there is nothing to worry. But that could also mean that the older OS'es define CC to point to older compilers...

I am leaning towards ignoring CC and removing that line from setup.py - is that fine?

lgarrison commented 3 years ago

Yes, that's fine with me!

On Fri, Jul 17, 2020 at 8:39 PM Manodeep Sinha notifications@github.com wrote:

May be modern OS's don't define CC and therefore there is nothing to worry. But that could also mean that the older OS'es define CC to point to older compilers...

I am leaning towards ignoring CC and removing that line from setup.py - is that fine?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manodeep/Corrfunc/pull/226#issuecomment-660394187, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLA7S4ATKOS2QGOQVF2RXDR4DVKJANCNFSM4OXDE6ZQ .

-- Lehman Garrison lgarrison@flatironinstitute.org Flatiron Research Fellow, Cosmology X Data Science Group Center for Computational Astrophysics, Flatiron Institute lgarrison.github.io

pep8speaks commented 3 years ago

Hello @manodeep! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-07-20 22:34:31 UTC
manodeep commented 3 years ago

@lgarrison I am done with this PR - will you please take a look? If you are satisfied, then I will merge this in and release 2.3.4