Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Use correct msvs language standard format #14

Closed
wants to merge 1 commit into from

Conversation

ianhattendorf
Copy link

@ianhattendorf ianhattendorf commented Mar 17, 2023

Note the `/` instead of `-` in `/std:c++17`.
@verhovsky
Copy link
Member

verhovsky commented Mar 19, 2023

Thanks for the suggestion. The CI fails for your PR because I deleted my fork of the superstring library from npm.

I think you're confusing two options, VCCLCompilerTool and LanguageStandard. In that thread the first suggestion is

['OS == "win"', {
  "msvs_settings": {
    "VCCLCompilerTool": {
      "AdditionalOptions": ["-std:c++17"],
    },
  },
}]

then there's a suggestion to use a completely different option (notice all 3 key names starting with msbuild_settings instead of msvs_settings are different)

['OS == "win"', {
  "msbuild_settings": {
    "ClCompile": {
      "LanguageStandard": "stdcpp17"
    }
  }
}]

then a third suggestion suggests changing - to / in the first suggestion

['OS == "win"', {
  "msvs_settings": {
    "VCCLCompilerTool": {
      "AdditionalOptions": ["/std:c++17"]
    }
  }
}]

I tried all 3 suggestions already actually, both the first and the third and they didn't work so I tried the second and committed that and that's the state of the repo you found when

in this PR you made a 4th suggestion, suggesting to change the syntax of the second suggestion, the one that uses a completely different option to

        ['OS == "win"', {
          "msbuild_settings": {
            "ClCompile": {
              "LanguageStandard": "/std:c++17"
            }
          }
        }]

and that of course fails with

error : Element <LanguageStandard> has an invalid value of "/std:c++17". [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]

https://github.com/curlconverter/node-tree-sitter/actions/runs/4461129448/jobs/7834740582

I have repeated the experiment with the 3rd suggestion (which is what I think you intended to suggest):

3rd suggestion: ceac04a

https://github.com/curlconverter/node-tree-sitter/actions/runs/4461203922/jobs/7834864609?pr=15#step:6:271

the 2nd suggestion: 0f9007f

https://github.com/curlconverter/node-tree-sitter/actions/runs/4461220528/jobs/7834890265?pr=15#step:6:219

and a run with no Windows-specific option at all: 3764003

https://github.com/curlconverter/node-tree-sitter/actions/runs/4461251085/jobs/7834945620?pr=15#step:6:271

First off, note that there's two CI jobs, the first does

npm install
npm test

on Node 14, 16 and 18. Additionally, on Node 14 we manually update node-gyp as suggested in this issue and it's the only Node version of the 3 that works, regardless of what's in binding.gyp, but those manual node-gyp update instructions don't work for Node 16 or 18.

and the second job does

npm install --ignore-scripts
npx --no-install prebuild <node targets>
npx --no-install prebuild <electron targets>

on Ubuntu, macOS and Windows. I have no idea why --ignore-scripts/--no-install is there and which scripts aren't running because of it, it was in the code I copied for this PR.

It's actually able to prebuild for all Node target versions and Electron target versions 11 and 12 and then it fails on 13. Neither suggestion 2 or 3 have any effect, we get the same result and error message when it's compiling win_delay_load_hook.cc
for Electron 13 when we don't include a Windows specific option at all. The only difference I can see is that adding "LanguageStandard": "stdcpp17" (the second suggestion) has this additional warning:

LINK : warning LNK4098: defaultlib 'LIBCMT' conflicts with use of other libs; use /NODEFAULTLIB:library [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]

which https://stackoverflow.com/questions/14148933/libcmt-conflicts-with-use-of-other-libs-unresolved-external-symbols says happens when

You are trying to compile with /MD, [...] but some code (probably one of the libraries) was built with /MT, and you can't have it both ways in the same program. You need to figure out which library was built with /MT and rebuild it with /MD.

Is nodejs/node-gyp#1686 (comment) relevant here?

@ianhattendorf
Copy link
Author

Ah yes, I meant approach 3. Odd, that's what we use and we're able to compile for Electron 21. We don't build prebuilts for Electron versions that are EOL so can't help with Electron 13. We default to C++17 on Node 18+/Electron 20+ otherwise default to C++14, might be worth trying?

https://github.com/nodegit/electron-npg-automator/actions/runs/4147287749/jobs/7173994146
https://github.com/nodegit/nodegit/blob/master/generate/templates/templates/binding.gyp
https://github.com/nodegit/nodegit/blob/master/utils/defaultCxxStandard.js

@verhovsky
Copy link
Member

I tried limiting it to only non-EOL Electron versions and still got the same error:

768f9a2

https://github.com/curlconverter/node-tree-sitter/actions/runs/4462402443/jobs/7836911199?pr=15

@verhovsky
Copy link
Member

verhovsky commented Mar 19, 2023

Looks like you're using a fork of nan, I wonder if that has anything to do with it.

https://github.com/nodegit/nodegit/blob/6ce1ebfe07f4f59b6aebacc127eff1b7cc9a7424/package.json#LL41C7-L41C18

https://github.com/axosoft/nan

@ianhattendorf
Copy link
Author

Looks like you're using a fork of nan, I wonder if that has anything to do with it.

That shouldn't affect this issue, that's just changing how nan accesses internal v8 fields to not conflict with cppgc.

I'm out of ideas, sorry :/

@verhovsky
Copy link
Member

verhovsky commented Mar 21, 2023

Alright, thanks for your help. I personally don't care about Windows+Electron, my library can't even be used on Electron anyway because I used top-level await (so that I wouldn't have to have to pass an un-initialized Parser object everywhere and make all of my libraries functions async, at least in the browser) and that requires your module to be an ESM module which Electron doesn't support. I wanted this to work to contribute it upstream instead of forking and other people are probably using tree-sitter in Windows+Electron, in fact the guy who wrote tree-sitter worked on/for the Atom editor, which is what Electron was originally made for...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants