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

Fix problem running 'make' with CC and CXX set #21985

Merged
merged 2 commits into from May 8, 2023

Conversation

mppf
Copy link
Member

@mppf mppf commented Mar 28, 2023

This PR is intended to resolve the build issue discussed in https://chapel.discourse.group/t/1-30-configuration-and-link-errors-when-building-on-system-where-1-29-previously-worked/20733/6

When building Chapel with something like CC=clang CXX=clang++ make, several things were going wrong:

  1. There was an error from printchplenv
  2. That error did not stop the build & later there were linker errors

Why was there an error at all? it was this sequence of events:

  • the Makefile invoking cmake set CMAKE_C_COMPILER and CMAKE_CXX_COMPILER based on CHPL_MAKE_HOST_CC etc to communicate this setting to cmake (so cmake does not infer it based upon CC or use its favorite compiler)
  • the cmake script invoked printchplenv while setting CHPL_HOST_CC and CHPL_HOST_CXX in the environment (so that printchplenv will be considering the same configuration that cmake is using)
  • printchplenv supports inferring CHPL_HOST_COMPILER from CC and CXX but not from CHPL_HOST_CC and CHPL_HOST_CXX. So, it failed, because the default value CHPL_HOST_COMPILER=gnu was inconsistent with CHPL_HOST_CC=clang / CHPL_HOST_CXX=clang++.

This PR:

  • It adjusts the cmake script to check for the return code from printchplenv and fail with an error if it is nonzero.
  • Enables inferring CHPL_HOST_COMPILER from CHPL_HOST_CC / CHPL_HOST_CXX
  • Enables inferring CHPL_TARGET_COMPILER from CHPL_TARGET_CC / CHPL_TARGET_CXX (but this does not happen when LLVM would be the default or when using a PrgEnv).

An alternative solution to the issue of inferring CHPL_HOST_COMPILER would be to pass it as an environment variable from make to cmake and from cmake to printchplenv.

Reviewed by @arezaii - thanks!

  • verified that CC=clang-14 CXX=clang++-14 make works on an Ubuntu 22.10 system where it did not work before
  • full local testing

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
@mppf
Copy link
Member Author

mppf commented Mar 28, 2023

@bradcray - this PR changes the inference we have talked about previously (#18450 and in discussion that led up to #17923). I'd like to know if you think the approach here is OK.

@bradcray
Copy link
Member

I'm OK with the approach at a high-level (particularly the stopping when printchplenv errors! :D ), but not invested enough in our interactions with cmake to be able to review that aspect of the approach or the implementation details.

Do any of these changes add new user-facing environment variables, or change the behavior of current ones? (I'm hoping not).

@mppf
Copy link
Member Author

mppf commented Mar 28, 2023

Do any of these changes add new user-facing environment variables, or change the behavior of current ones? (I'm hoping not).

Yes, these parts are new and user-visible:

  • Enables inferring CHPL_HOST_COMPILER from CHPL_HOST_CC / CHPL_HOST_CXX

  • Enables inferring CHPL_TARGET_COMPILER from CHPL_TARGET_CC / CHPL_TARGET_CXX (but this does not happen when LLVM would be the default or when using a PrgEnv).

I view these as a relatively straightforward generalization of how we can infer CHPL_HOST_COMPILER from CC. Previously to this PR, we infer from CC/CXX in certain settings; but this PR adds inferring the compiler family from the _HOST_ or _TARGET_ versions of CC/CXX.

I'd consider that a behavior change.

The PR doesn't add any new Chapel configuration variables.

If this direction gives you a lot of pause, there are certainly other ways we could fix the original issue.

@bradcray
Copy link
Member

So CHPL_HOST_CC / CHPL_TARGET_CC were supported and documented before, but if the user set them, they did not lead to changes in CHPL_HOST_COMPILER/CHPL_TARGET_COMPILER? What was the rationale for that? What did setting them do? (checking the docs, it's not completely clear to me—my guess: they specified which compiler to use, but simply didn't affect the compiler family?). If I've got that all straight, this change seems sensible/obvious to me.

Is it correct that if both CHPL_HOST_CC and CC are set, CHPL_HOST_CC wins? (and ditto for TARGET modulo the PrgEnv / LLVM caveats?). And that if CHPL_HOST_COMPILER is set and one of the CC variables is set, the CC variable is used, but with the compiler family settings (Makefiles, flags, etc.) specified by the CHPL_HOST_COMPILER setting? Also sounds good to me if I've got it right.

@mppf
Copy link
Member Author

mppf commented Mar 28, 2023

So CHPL_HOST_CC / CHPL_TARGET_CC were supported and documented before, but if the user set them, they did not lead to changes in CHPL_HOST_COMPILER/CHPL_TARGET_COMPILER?

Yes, the user was expected to set all of the relevant variables (e.g. CHPL_HOST_CC, CHPL_HOST_CXX, CHPL_HOST_COMPILER).

What was the rationale for that?

I do not know. It might have been an oversight. I don't know of a rationale to not infer CHPL_HOST_COMPILER from CHPL_HOST_CC e.g.

What did setting them do? (checking the docs, it's not completely clear to me—my guess: they specified which compiler to use, but simply didn't affect the compiler family?).

Setting e.g. CHPL_HOST_CC you can indicate which command to use to compile without changing the compiler family. That leads to errors if it does not match what was otherwise provided / inferred for CHPL_HOST_COMPILER.

If I've got that all straight, this change seems sensible/obvious to me.

👍

Is it correct that if both CHPL_HOST_CC and CC are set, CHPL_HOST_CC wins? (and ditto for TARGET modulo the PrgEnv / LLVM caveats?).

Yes, we don't consider using CC / CXX at all if any of CHPL_HOST_CC / CHPL_HOST_CXX / CHPL_HOST_COMPILER / CHPL_TARGET_CC / CHPL_TARGET_CXX / CHPL_TARGET_COMPILER are set. (and this is the documented behavior).

And that if CHPL_HOST_COMPILER is set and one of the CC variables is set, the CC variable is used, but with the compiler family settings (Makefiles, flags, etc.) specified by the CHPL_HOST_COMPILER setting? Also sounds good to me if I've got it right.

CC won't be used if CHPL_HOST_COMPILER is set per the documentation (link above). That existed before this PR and I didn't change it here.

@bradcray
Copy link
Member

That all sounds good to me — thanks for the clarifications and reminders! (note that I have still not done anything to review the PR itself, just the behavior—and hope not to have to).

@bradcray
Copy link
Member

Just catching up on email and noting that this could probably be merged now (?). (No surprise it hasn't been given the last few weeks, though).

@mppf mppf merged commit 4112d47 into chapel-lang:main May 8, 2023
7 checks passed
@mppf mppf deleted the fix-cc-setting-issue branch May 8, 2023 14:45
mppf added a commit that referenced this pull request May 9, 2023
Follow-up to #21985 to resolve a Cygwin configuration failure where
`CHPL_HOST_CC` is set to `gcc.exe` and the compiler detection logic is
unable to infer `CHPL_COMPILER=gnu` from that.

Reviewed by @arezaii - thanks!

- [x] `make && make check` succeeds on a Cygwin system
- [x] full comm=none testing
mppf added a commit that referenced this pull request Jun 13, 2023
Docs follow-up to PR #21985.

This PR updates the chplenv.rst documentation to better describe the
default of `CHPL_HOST_COMPILER` and `CHPL_TARGET_COMPILER` and to add a
new section about `CC` and related variables.

Reviewed by @arezaii - thanks!
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