[vcbuild.bat] bug fix & add ccache support #52126
Conversation
msbuild property need pass by /p:key=value
nodejs-github-bot
added
build
congzhangzh
changed the title
|
@zcbenz @liudonghua123 @targos Hi folks, can you do a code review:) |
|
cc @nodejs/platform-windows |
|
Hello @congzhangzh. Changes from this PR interest me. I've tried running it locally and got the compilation to pass, but regarding Can you share the setup you're using eg. ccache configuration and other prerequisites? Thanks in advance. |
@StefanStojanovic Hi Stefan, can you try to do test in a small project first, follow, copy ccache.exe as cl.exe, I assume you install cache to c:\ccachecreate a Directory.Build.props in you project dir or upper diror if it still not workTips: Microsoft's document confused me, they say you can not use Directory.Build.props to override some C++ settings like ClCompile part, but I works for me, for 100% work, I split it into different files to be sure it always works! props_4_ccache.props<Project>
<!-- The following will set the needed properties described in Tips above. -->
<!-- Creation of a precompiled header will have to be disabled separately. -->
<ItemDefinitionGroup>
<ClCompile>
<DebugInformationFormat>OldStyle</DebugInformationFormat>
<ForcedIncludeFiles />
<ObjectFileName>$(IntDir)%(FileName).obj</ObjectFileName>
<PrecompiledHeader>NotUsing</PrecompiledHeader>
</ClCompile>
</ItemDefinitionGroup>
</Project>BTW, I do many ccache debugging these days, feedback is very welcome, the latest document in ccache is updated by me, some may be misleading if I understand wrong! |
|
BTW, I am 100% sure ccache works for me on windows |
|
@congzhangzh I installed ccache with Chocolatey, so for me it is in Anyway, I tried what you suggested (split |
|
@StefanStojanovic Hi Stefan, Can you try download ccache to ccache, and don’t install chocolatey, this may simple the debug process, Btw, Can you show ccache -s and ccache —print-state here? you can use https://learn.microsoft.com/en-us/sysinternals/downloads/process-explorer to get how ccache dummy cl& real cl be called too:) |
|
Hey @congzhangzh it worked when using ccache directly, not via Chocolatey. Thanks for your help. I want to enable using ccache in Node.js CI, and for that, Chocolatey would be great. I'll investigate the issues with that approach further. |
@StefanStojanovic Hi Stefan, you should not use shimed ccache shim executable, but the the real file the document https://github.com/ccache/ccache/wiki/MS-Visual-Studio point that, you can: copy C:\ProgramData\chocolatey\lib\ccache\tools\ccache-4.7.4-windows-x86_64\ccache.exe C:\some\folder\cl.exeBTW, some of the ccache document update by me recently, you may need double check:) |
Thanks for the heads up @congzhangzh. I saw that Chocolatey's using shimmed ccache and that's a problem. Still, on the other hand, Chocolatey gives us a simple mechanism for updating installed packages (we already leverage this in the CI). Since it downloads the actual ccache.exe, I can copy it as |
|
@StefanStojanovic Hi Stefan, I create a new pull request focus on ccache 4 windows |
| <ObjectFileName>$(IntDir)%(FileName).obj</ObjectFileName> | ||
| </ClCompile> | ||
| </ItemDefinitionGroup> | ||
| </Project> No newline at end of file |
There was a problem hiding this comment.
Could you add an empty line at the bottom?
Also, I think a better place for this file would be in tools/msvs as there are all Windows-specific tools. Once you move it, just make sure to change the path in ForceImportAfterCppProps in vcbuild.bat too.
| set "extra_msbuild_args=%extra_msbuild_args% /p:UseMultiToolTask=True" | ||
| set "extra_msbuild_args=%extra_msbuild_args% /p:EnforceProcessCountAcrossBuilds=True" | ||
| set "extra_msbuild_args=%extra_msbuild_args% /p:MultiProcMaxCount=%NUMBER_OF_PROCESSORS%" | ||
| set "extra_msbuild_args=%extra_msbuild_args% /p:VcpkgEnabled=false" |
There was a problem hiding this comment.
This line was not here before (the other 3 are rewritten). Is this connected with your other PR?
There was a problem hiding this comment.
Partially, if this pull request is approved, the https://github.com/nodejs/node/pull/52181/files will be upgraded to use vcbuild ccache switch directly:)
I'm a little worried, this pr changed too much, which is hard to accept, so I decided to split it out or drop this pull request, but use cmdline to config msbbuild directly, which is enough to make it work.
Btw, I don't like vcbuild.bat, why not use configure.py directly, The windows support for developing node itself is not so good, maybe most guys use linux/mac:)
There was a problem hiding this comment.
BTW, maybe that's my fault, it seems just setting environment variable is enough, I will check it later
There was a problem hiding this comment.
@StefanStojanovic Hi Stefan, sorry about my fault, it works by setting msbuild property directly
I do a test
set TrackFileAccess=false
set UseMultiToolTask=true
: allow link symbols redefine
set ForceImportAfterCppTargets=some_build.targets
msbuild ConsoleApplication_with_symbol_redefine.slnand it works!
|
let's move to #52255 |
4 participants
msbuild property need pass by /p:key=value