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

c_cpp_properties.json is now requiring write access #1790

Closed
daltairwalter opened this issue Apr 4, 2018 · 16 comments
Closed

c_cpp_properties.json is now requiring write access #1790

daltairwalter opened this issue Apr 4, 2018 · 16 comments
Labels
bug fixed Check the Milestone for the release in which the fix is or will be available. help wanted Can be fixed in the public (open source) repo. Language Service
Milestone

Comments

@daltairwalter
Copy link

The c_cpp_properties.json now requires write access or it will not parse. I am using the C/C++ extension 0.16.1 and the vscode version:
Version 1.21.1
Commit 79b44aa704ce542d8ca4a3cc44cfca566e7720f1
Date 2018-03-14T14:46:47.128Z
Shell 1.7.9
Renderer 58.0.3029.110
Node 7.9.0
Architecture x64

We use perforce and I had to check the file out before it would read correctly.
Without this file checked out I get the error:
Failed to parse "h:....vscode\c_cpp_properties.json'

This is a new problem as we have been saving this file in perforce for a while.

@bobbrow
Copy link
Member

bobbrow commented Apr 4, 2018

In this latest version, we wanted to show customers which compiler we were using to populate the system include path and defines as well as which c++ standard IntelliSense was using.

We haven't added any new settings to the c_cpp_properties.json schema in a while. But since we are an extension still in preview and are trying to improve the IntelliSense configuration experience, we do attempt to update existing c_cpp_properties.json files from time to time. We should not make it mandatory to write back to the file though. We should be able to get by with doing this in memory and recomputing the changes if you edit the file again.

@bobbrow bobbrow added bug Language Service help wanted Can be fixed in the public (open source) repo. labels Apr 4, 2018
@sean-mcmanus
Copy link
Collaborator

Is checking the file out, letting the extension modify the file, and then checking the changes back in a workaround? We try to add cppStandard and cStandard values, but I don't think we do that repeatedly if they're already there. On Linux/Mac we try to add compilerPath if a default is found.

But we could add code to check if the file isn't writeable too.

@pyrrho
Copy link

pyrrho commented Apr 5, 2018

Having hit a similar issue, I considered checking the automatically generated changes into git, but not knowing how it will interact with different compiler paths across different computers gives me pause. I've had to switch back and forth between my system clang and a locally-built copy of GCC a fair few times now, so I'm not actually sure that workaround will work for me locally, long-term.

Even if it will work, the idea of committing a system-generated, absolute path into a shared configuration file doesn't sit terribly well with me.

@ghost
Copy link

ghost commented Apr 5, 2018

I also hit an issue with VS Code repeatedly updating the c_cpp_properties.json file if it didn't contain the cppStandard and cStandard fields.

This kept me from using certain git commands while VS Code was open (because the working tree wasn't clean). If I stashed the changes to this file, VS Code would just add them back.

As a workaround I just ended up committing the changes to the repo, but I feel like VS Code shouldn't modify this file on its own..

@akoeplinger
Copy link
Member

I'm hitting this file being modified now as well and while checking in the cppStandard and cStandard fields sounds OK doing the same for compilerPath does not as it can be different on other machines.

@sean-mcmanus is setting "compilerPath": "" a good workaround or does it have unintended implications?

@bobbrow
Copy link
Member

bobbrow commented Apr 9, 2018

Setting the compiler path to "" turns off the system include and preprocessor symbol discovery logic.

However, a feature I am working on may provide the solution. We're going to add VSCode settings for all of the properties of c_cpp_properties.json so that you can set default values for them and not need to set them in c_cpp_properties.json anymore. I'm working on the writeup for this which I will post in #1338 if you would like to review it and provide feedback.

@akoeplinger
Copy link
Member

@bobbrow sounds good but I think there needs to be a short-term fix for this is well, e.g. what you mentioned in #1790 (comment)

@bobbrow bobbrow modified the milestones: May 2018, April 2018 Apr 9, 2018
@bobbrow
Copy link
Member

bobbrow commented Apr 9, 2018

Absolutely, though I think both of these will come out in the same release (the next one). I'll mark it for the same milestone so I don't lose track of it.

bobbrow added a commit that referenced this issue Apr 9, 2018
bobbrow added a commit that referenced this issue Apr 10, 2018
* addresses #1599, #1790

* Update all configs when c_cpp_properties.json is parsed
@sean-mcmanus sean-mcmanus added the fixed Check the Milestone for the release in which the fix is or will be available. label Apr 14, 2018
@bobbrow
Copy link
Member

bobbrow commented Apr 24, 2018

A preview of this is available in the insiders build if you would like to try it out and provide feedback.

@akoeplinger
Copy link
Member

@bobbrow thanks, I tried this build but it still writes the fields to the config file all the time.

@bobbrow
Copy link
Member

bobbrow commented Apr 24, 2018

Have you disabled write-access to the file? We will still attempt to write to the file, but we shouldn't crash if the write fails.

@akoeplinger
Copy link
Member

@bobbrow no I haven't and I don't see why I should. Those fields should only be written if they don't have the default value.

@bobbrow
Copy link
Member

bobbrow commented Apr 24, 2018

Given the number of configuration issues we've serviced, we are attempting to make it clearer to developers how IntelliSense is configured. And if we don't expose the new properties in some way, people are unlikely to find them.

However, I can move this logic into a configuration upgrade block so that we only attempt to add them once.

@akoeplinger
Copy link
Member

akoeplinger commented Apr 24, 2018

Since my main problem with writing to the file is that this is checked into git and shared between colleagues (so hardcoding "compilerPath": "/usr/bin/clang" into the file can't be done) I think using ${default} from #1338 will solve the issue much better for me.

Though I should note that #1338 (comment) mentions:

Is omitting a configuration option the same as specifying it to ${default}?

Yes.

... which doesn't seem to happen for me, if I omit the compilerPath and save the file just gets updated with hardcoded /usr/bin/clang.

@bobbrow
Copy link
Member

bobbrow commented Apr 24, 2018

Yeah, compilerPath, cStandard, and cppStandard all keep coming back, but the other properties do not. If I move the code that adds these into an upgrade block, then it will only happen once and leave you alone if you want to delete it later.

@bobbrow
Copy link
Member

bobbrow commented May 8, 2018

This is fixed in 0.17.0.

In addition, compilerPath, cStandard, cppStandard, and intelliSenseMode will only be force-written to c_cpp_properties.json one last time during the config upgrade to version 4. You should be able to permanently delete them after that if you want to use the defaults for your platform or the new default settings we added to the extension.

@bobbrow bobbrow closed this as completed May 8, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fixed Check the Milestone for the release in which the fix is or will be available. help wanted Can be fixed in the public (open source) repo. Language Service
Projects
None yet
Development

No branches or pull requests

5 participants