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

Respect CC & CXX env variables to set compilers #41

Merged
merged 1 commit into from
May 15, 2023

Conversation

smtrfnv
Copy link
Contributor

@smtrfnv smtrfnv commented May 9, 2023

  • If CC and CXX environment variables are set then use them as paths to compilers.
  • fix minor warnings appeared when clang++-15 was used
  • remove not used compiler option from the configuration script

@smtrfnv smtrfnv marked this pull request as draft May 9, 2023 13:19
@smtrfnv smtrfnv marked this pull request as ready for review May 9, 2023 13:24
@smtrfnv smtrfnv marked this pull request as draft May 9, 2023 18:40
@smtrfnv smtrfnv marked this pull request as ready for review May 9, 2023 18:43
Copy link
Contributor

@mattrm456 mattrm456 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could revise this PR to allow the user to solely set CC or CXX to a path where we find a compiler whose basename we recognize and set the other language compiler accordingly, if unset. For example if CXX is "/usr/local/bin/xlC_r" and CC is unset then we treat CC as if its set to /usr/local/bin/xlc. I can't imagine a scenario where one would want to use g++ for compiling C++ and clang for compiling C, to be linked together. This would be a small convenience feature so that users don't have to set both environment variables. For explicitly set paths to compilers whose basenames we don't recognize we'd treat both environment variables as mandatory.

That being said, in my experience it is uncommon for open source projects I've built to support such a convenience, and for the goal of this PR this proposed implementation is satisfactory. I'm leaving this comment for posterity in case we revisit this area in the future. Leave this PR as-is for now.

OK to merge.

@mattrm456 mattrm456 merged commit ab6f596 into bloomberg:main May 15, 2023
@smtrfnv smtrfnv deleted the respect_cc_cxx branch November 2, 2023 12:46
che2 pushed a commit to che2/ntf-core that referenced this pull request Aug 14, 2024
lxv pushed a commit to lxv/ntf-core that referenced this pull request Dec 19, 2024
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.

2 participants