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

Adding support for Visual Studio Code #2168

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kino-dome
Copy link
Contributor

Added VS Code workspace files to Cinder and all of the samples. Updated Cmake guide to introduce VS Code as a free cross-platform IDE solution and how to use it along with cmakeTools and cppTools extensions in VS Code

Added VS Code workspace files to Cinder and all of the samples. Updated Cmake guide to introduce VS Code as a free cross-platform IDE solution and how to use it along with cmakeTools and cppTools extensions
@kino-dome
Copy link
Contributor Author

As per Rich's suggestion, found a way to pack the needed settings in the code-workspace file and ditch the hidden .vs-code folder all together.

@kino-dome
Copy link
Contributor Author

kino-dome commented May 18, 2020

Note: For the vs-code files to work on Windows (with Visual Studio 2019 toolchain), PRs #2165 , #2166 and #2167 need to be merged first. You can try the whole thing at the mMaster branch on my fork.

@kino-dome
Copy link
Contributor Author

P.S : I haven't tested this on osx yet because I don't have one around but it should work OK if cmake was already working on it. It'll be great if someone can test this. Maybe with the addition of this?

Corrected the info on configuring Cinder in the guide
@MikeGitb
Copy link
Contributor

Still think it would be much more effective and efficient to put the cmake files into the root folder where they belong instead of adding N tool specific configuration files to each of the M examples and maintain them, but I guess that needs a decision from @richardeakin or @andrewfb.

@kino-dome
Copy link
Contributor Author

Still think it would be much more effective and efficient to put the cmake files into the root folder where they belong instead of adding N tool specific configuration files to each of the M examples and maintain them, but I guess that needs a decision from @richardeakin or @andrewfb.

Yeah I guess that's something best decided by the main maintainers of Cinder since it will be a breaking change and previous projects will need to be modified if this change occurs. Meanwhile I need to say that there is no more the need of having N configuration files because I managed to put all the necessary settings in the code-workspace file itself. So with the current PR, there is only one vs-code workspace file added for each project and that's it.

Also the needed settings are more than just pointing to the CML file in my case, so even if the CML files are moved to the root of each project, there still needs to be other settings set in the project file. To demonstrate this, here's the content of the code-workspace file for libcinder itself (where the CML file is on the root):
{ "folders": [ { "path": "../../." } ], "settings": { "C_Cpp.default.configurationProvider": "vector-of-bool.cmake-tools", "cmake.configureSettings": {"BUILD_SHARED_LIBS":false} } }

And here it is for a sample that has the CML in proj/cmake:
{ "folders": [ { "path": "../../." } ], "settings": { "C_Cpp.default.configurationProvider": "vector-of-bool.cmake-tools", "cmake.sourceDirectory": "${workspaceFolder}/proj/cmake", "cmake.buildDirectory": "${workspaceFolder}/proj/cmake/build" } }

As you can see the settings are more than the path to the CML and will need to be there anyways. Although as mentioned, I don't think that's a problem in either case because all is embedded in the workpspace file itself

@kino-dome
Copy link
Contributor Author

One question I had when it comes to the discussion about file paths, is the location of these code-workspace files. For now I have put them in /proj/cmake because I thought since they are associated with Cmake, it's best to keep them in the same folder. I'm not sure about this though and could use some feedback here. Maybe putting them in the /proj folder makes more sense because it'll be one step above all the cmake config files, what do you think guys? Or maybe even adding them to a respective vs-code folder?

@MikeGitb
Copy link
Contributor

The N was refererring to the N different tools that users might want to use Cinder with.

@MikeGitb
Copy link
Contributor

And I don't think any of those additional settings are actually necessary in order to allow users to interact with cinder in a convenient manner.

@MikeGitb
Copy link
Contributor

E.g.:

  • why does the output have to be in "${workspaceFolder}/proj/cmake/build" ? Instead of whatever VSCode picks as default (${workspaceFolder}/build in my case)
  • "C_Cpp.default.configurationProvider": "vector-of-bool.cmake-tools": Is it necessary and is that even up to date (given that cmake-tools are now maintained by MS)? On my system VSCode seems to pick cmake-tools by default. I can't say if I configured it that way sometime in the past or if that is the default behavior, but if a user is using VSCode for c++ development, I'd expect them to know how to use it with a cmake-based project no?
  • "cmake.configureSettings": {"BUILD_SHARED_LIBS":false}: Is that necessary? I think it is the default anyway and if not, you can just set it in the CML no?

@kino-dome
Copy link
Contributor Author

The N was referring to the N different tools that users might want to use Cinder with.

Oops sorry, thought you meant the files for each tool.

why does the output have to be in "${workspaceFolder}/proj/cmake/build" ? Instead of whatever VSCode picks as default (${workspaceFolder}/build in my case)

I followed Cinder's own routine. Since the CML files are inside /proj/cmake, Cmake builds the folder in that folder as well. I just set the settings in a way that the same thing happens in vs-code instead of having the build folder created on root. To keep everything consistent with Cinder's way of doing things I suppose.

"C_Cpp.default.configurationProvider": "vector-of-bool.cmake-tools": Is it necessary and is that even up to date (given that cmake-tools are now maintained by MS)? On my system VSCode seems to pick cmake-tools by default. I can't say if I configured it that way sometime in the past or if that is the default behavior, but if a user is using VSCode for c++ development, I'd expect them to know how to use it with a cmake-based project no?

If this value is not set, the first time the workspace project is opened, vs-code asks if it should use the Cmake toolchain as the C++ configuration provider. If answered yes by the user, the exact same setting is written to a settings.json file in a hidden .vs-code folder on ${workspaceFolder}. So to answer your question, yes this is necessary and it should be either set by the user in the workspace file or set by vs-code after the prompt. I chose to do the former, again to keep everything organized in one file except for having an extra hidden folder lying around on root.
Also as to your point about cmake-tools being maintained by MS, that's true but the name referred to this extension internally, is still the same one as before vector-of-bool.cmake-tools. I guess they kept that to prevent any breaking changes.

"cmake.configureSettings": {"BUILD_SHARED_LIBS":false}: Is that necessary? I think it is the default anyway and if not, you can just set it in the CML no?

Yes, you can do that but that's not the recommended way when it comes to vs-code and cmake-tools. Have a look at this, here it explicitly says that to change configuration settings, it's best to set this value in the settings.json file. And again to prevent a redundant hidden folder, I decided to embed this setting in the workspace file itself.

@MikeGitb
Copy link
Contributor

In all the points I was referring to your comment that even if the CML was in the root, a VS-Code config file was needed

I followed Cinder's own routine. Since the CML files are inside /proj/cmake, Cmake builds the folder in that folder as well. I just set the settings in a way that the same thing happens in vs-code instead of having the build folder created on root. To keep everything consistent with Cinder's way of doing things I suppose.

That's only a reason as long as the CML is in that sub directory instead of the root.

If this value is not set, the first time the workspace project is opened, vs-code asks if it should use the Cmake toolchain as the C++ configuration provider. 

I haven't been asked this on my machine when I moved the CML into the root - it just worked, but even if it happens- so what?

So to answer your question, yes this is necessary and it should be either set by the user in the workspace file or set by vs-code after the prompt.

Sorry If I was unclear: I was asking if it is necessary to set this option in a cinder-provided file. If VSCode creates it itself after a simple yes/no question, I'd say no, it is not necessary.

Yes, you can do that but that's not the recommended way when it comes to vs-code and cmake-tools. 

I think you ignored the first part of my question: Isn't that the default anyway?

Have a look at this, here it explicitly says that to change configuration settings, it's best to set this value in the settings.json file

It says that IF you need to set options, you shouldn't do it via -D.... but via their own key : value syntax.

a) That doesn't mean you shouldn't set necessary options and flags in the CML (which btw. has the advantage that they are now picked up by all editors- not just VSCode) and

b) I don't see, why this option has to be set explicitly at all given that building static libs is the default behavior anyway.

Again, my argument here is that, if the existing CMLs would be moved to the root, everything would probably just work for a number of different editors (including VSCode, but also VS, QtCreator, CLion and probably a few more) without adding any tool specific files. At worst the user might have to answer 1-2 popup dialogs (again, I didn't have to, but that might be specific to my environment).

If the project maintainers instead decide that they want to keep the CMLs in the sub directory and add individual support for various tools, then this looks like a good PR to add support for VSCode.

@PetrosKataras
Copy link
Contributor

I believe that the original motivation for having separate folders for the different editors/toolsets that Cinder supports on the different platforms was mainly to keep different project files/directories isolated/uncluttered and easy for the user to pick and start depending on the development platform at hand.

With support for VS and Xcode project files along with CMake there had to be some directory restructuring to keep things tidy and at that point, it felt that keeping the CMake dir structure similar as to the other tools was a natural choice.

I understand that this is a bit in contrast with the traditional CMake approach of having things located at the root folder but on the other hand I do not personally see a real issue with the current structure and people are already using different editors with the current CMake configuration with minimal setup needed. That said, with the number of different editors supporting CMake increasing constantly, I think it would be problematic to try and follow a similar pattern for each individual editor.

In the case of VSCode ( and any other editor ) I suppose the main question would be what is the maintenance cost and how can this be minimised - Considering that, personally I would lean towards @MikeGitb direction in that when it comes to support for different editors whatever can be set by user or defaults it is better to stay like this instead of adding individual settings files since this would provide the path of least resistance moving forward.

If something doesn't work this way because of Cinder's CMake configuration then we should try to solve the issue there ( instead of patching the issue on the editor settings side ) so that other editors can benefit by default also.

If there is a real explicit need for some editor settings to be stored and its not just about conveniency or saving clicks then I guess it is really a question of the maintenance cost/benefit ratio at the end of the day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants