Skip to content

Conversation

@JoaoPauloCMarra
Copy link

Ensure CC and CXX environment variables are set for the Hermes
external project to match the main project's compilers. This fixes
linking issues where the wrong compiler toolchain was being used
for the final link step.

test env:

sw_vers
ProductName:		macOS
ProductVersion:		26.1
BuildVersion:		25B78

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses compiler toolchain mismatch issues on macOS by ensuring the Hermes external project uses the same C and C++ compilers as the main project. The fix adds environment variable settings to propagate the compiler configuration.

  • Added environment variable settings for CC and CXX in the Hermes ExternalProject_Add configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +110 to +111
ENV CC ${CMAKE_C_COMPILER}
ENV CXX ${CMAKE_CXX_COMPILER}
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The ENV directive syntax is incorrect. Each ENV entry should be a single key=value pair on one line. The correct syntax should be ENV \"CC=${CMAKE_C_COMPILER}\" \"CXX=${CMAKE_CXX_COMPILER}\" or split into separate ENV directives like ENV CC=${CMAKE_C_COMPILER} and ENV CXX=${CMAKE_CXX_COMPILER}.

Suggested change
ENV CC ${CMAKE_C_COMPILER}
ENV CXX ${CMAKE_CXX_COMPILER}
ENV "CC=${CMAKE_C_COMPILER}" "CXX=${CMAKE_CXX_COMPILER}"

Copilot uses AI. Check for mistakes.
Comment on lines +110 to +111
ENV CC ${CMAKE_C_COMPILER}
ENV CXX ${CMAKE_CXX_COMPILER}
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

These environment variables appear redundant since CMAKE_C_COMPILER and CMAKE_CXX_COMPILER are already being passed via CMAKE_ARGS at lines 90-91. While setting both CMAKE_ARGS and ENV variables can help with multi-stage builds (configure vs build/link), consider documenting why both are necessary if this duplication is intentional.

Suggested change
ENV CC ${CMAKE_C_COMPILER}
ENV CXX ${CMAKE_CXX_COMPILER}

Copilot uses AI. Check for mistakes.
@tmikov
Copy link
Owner

tmikov commented Nov 12, 2025

I agree with Copilot. Why do we need to set the environment variables if we are already setting the CMake defines?

@JoaoPauloCMarra JoaoPauloCMarra deleted the fix-hermes-compiler-env branch November 12, 2025 17:20
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