Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed compile failure for missing 'CC' key #226

Merged
merged 6 commits into from
Jul 21, 2020
Merged

Fixed compile failure for missing 'CC' key #226

merged 6 commits into from
Jul 21, 2020

Conversation

manodeep
Copy link
Owner

Fix for #225

@manodeep manodeep added this to the v2.3.4 milestone Jul 11, 2020
@manodeep
Copy link
Owner Author

@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?

@manodeep manodeep self-assigned this Jul 17, 2020
@manodeep manodeep requested a review from lgarrison July 17, 2020 04:14
@lgarrison
Copy link
Collaborator

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

manodeep commented Jul 18, 2020

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

lgarrison commented Jul 18, 2020 via email

@pep8speaks
Copy link

pep8speaks commented Jul 19, 2020

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! 🍻

Comment last updated at 2020-07-20 22:34:31 UTC

@manodeep
Copy link
Owner Author

@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

Removed misleading comment that environment 'CC' was honoured
@manodeep manodeep merged commit ad74816 into master Jul 21, 2020
@manodeep manodeep deleted the fix_env_cc branch July 21, 2020 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants