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

feat(msvs): add support for CL env var #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mscdex
Copy link

@mscdex mscdex commented Nov 3, 2021

This is probably not the best place for this line to be as I'm not a gyp expert, but it works for me. I'm completely open to suggestions if anyone has better ideas on how to achieve this.

My use case is I wanted to easily set some compiler flags (e.g. _WIN32_WINNT) without having to touch every gyp/gypi file in a project. On *nix you can simply set the CFLAGS/CXXFLAGS/etc. environment variables and it'll just work.

However on Windows it's a different story. Technically you can set a CL environment variable to pass flags to CL.exe, but since gyp uses msbuild, environment variables like CL don't get picked up/used automatically. Because of this, we need to explicitly pass the values to the msbuild project file.

I tried looking to see if node-gyp/gyp had some way of achieving the same thing using the command line, but I could not find anything, so using environment variables seems like the next best bet.

@mscdex
Copy link
Author

mscdex commented Nov 3, 2021

Also, I'm not entirely sure but "cl" might need to also be added to msvs_emulation.py in the environment variable list in _ExtractImportantEnvironment. It appears that file is mostly for ninja builds, which I can't test right now.

@cclauss
Copy link
Contributor

cclauss commented Nov 3, 2021

Please add doctests to this function. Something like:

def _GetValueFormattedForMSBuild(tool_name, name, value):
    """
    >>> _GetValueFormattedForMSBuild(
    ...     tool_name="tool_name", name="name", value=[]
    ... )
    ''
    >>> _GetValueFormattedForMSBuild(
    ...     tool_name="tool_name", name="name", value=["DelayLoadDLLs"]
    ... )
    'DelayLoadDLLs'
    >>> _GetValueFormattedForMSBuild(
    ...     tool_name="Lib", name="AdditionalOptions", value=["DelayLoadDLLs"]
    ... )
    'DelayLoadDLLs %(AdditionalOptions)'
    >>> _GetValueFormattedForMSBuild(
    ...     tool_name="ClCompile", name="AdditionalOptions", value=["DelayLoadDLLs"]
    ... )
    'DelayLoadDLLs %(AdditionalOptions)'
    """

@mscdex
Copy link
Author

mscdex commented Nov 5, 2021

@cclauss Added.

@mscdex
Copy link
Author

mscdex commented Nov 6, 2021

It looks like the unrelated windows integration failure is because node is floating changes on top of gyp that do not exist in node-gyp's vendored copy of gyp or in gyp-next itself. Specifically, gyp isn't setting PRODUCT_DIR_ABS. The use of this variable started with the landing of OpenSSL 3.0 I believe.

@@ -3440,6 +3440,24 @@ def _FinalizeMSBuildSettings(spec, configuration):


def _GetValueFormattedForMSBuild(tool_name, name, value):
"""
>>> _GetValueFormattedForMSBuild(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
>>> _GetValueFormattedForMSBuild(
>>> PRODUCT_DIR_ABS =PRODUCT_DIR_ABS
>>> _GetValueFormattedForMSBuild(

@targos
Copy link
Member

targos commented Nov 6, 2021

It looks like the unrelated windows integration failure is because node is floating changes on top of gyp that do not exist in node-gyp's vendored copy of gyp or in gyp-next itself. Specifically, gyp isn't setting PRODUCT_DIR_ABS. The use of this variable started with the landing of OpenSSL 3.0 I believe.

I opened nodejs/node#40735

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