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

Add devcontainer & simplify quick start #1

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

Conversation

kmichaelk
Copy link
Contributor

@kmichaelk kmichaelk commented Oct 9, 2024

Using a container for development will help to avoid the hassle of installing dependencies in the global space and dealing with version conflicts, especially CMake, the version of which on the current LTS version of Ubuntu is below the minimum required (3.22 < 3.25). Devcontainer support is implemented in all the most popular C++ IDEs, including Visual Studio, VSCode and CLion, Windows users can leverage it via WSL2.

The directory with the VSCode workspace configuration is included in version control with the required CMake configuration flags and a proposal to install clangd language server, which also supports clang-format - for ease of use, files will be automatically formatted when saving. As it partially embeds clang-tidy, some checks will also be run, reducing the need to wait for CI checks when not necessary and abuse it ultimately.

PR also includes minimal opinionated VSCode configuration for convenience: filtered seq test running, debugging, etc.

@kmichaelk kmichaelk force-pushed the vsc-devcon branch 3 times, most recently from af2d95d to 73edf1e Compare October 10, 2024 21:37
@allnes allnes requested review from allnes and aobolensk October 10, 2024 22:51
@kmichaelk kmichaelk changed the title Add devcontainer Add devcontainer & simplify quick start Oct 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.98%. Comparing base (27dc06d) to head (bb05888).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
+ Coverage   94.92%   94.98%   +0.06%     
==========================================
  Files          28       28              
  Lines         650      658       +8     
  Branches      225      222       -3     
==========================================
+ Hits          617      625       +8     
  Misses         19       19              
  Partials       14       14              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@khasanyanovk khasanyanovk left a comment

Choose a reason for hiding this comment

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

really usefull method to build and edit code
like it

@aobolensk
Copy link
Member

The idea of this change is good, but there are two points:

  1. please add CI verification for dev container
  2. Excluding .vscode from gitignore and enforcing your settings for everyone is not very good idea. I'd suggest to avoid this

@kmichaelk
Copy link
Contributor Author

kmichaelk commented Oct 21, 2024

I agree that including .vscode in version control is a debatable thing, but this is also the practice of many large open source projects (for which VSCode is a common IDE) to share task configurations, recommended extensions, and enforce a common configuration for the workspace - it rarely needs to be changed because in most cases people change the settings in the User scope, not the Workspace - in the few cases where it is necessary to change the Workspace configuration, these changes are important to everyone and have a rationale, so they are also committed to the repository, i.e. an important point of workspace configurations is to share them.

The configuration is minimal actually - recommendations for installing a couple of extensions, in addition to those installed during the installation of the devcontainer environment, tasks for building each target, and a small config that disables IntelliSense provided by Microsoft C++ LSP Server in favor of clangd (Microsoft's language server is still needed as it provides gdb support) i and enforces formatting when saving using clang-format and a bunch of settings for the test runner that integrates with the VSCode UI and allowing you to run tests from the sidebar or directly from their definition, which makes development more productive.

The only concern is the possible desire to change the CMake configuration arguments in the settings.

TL;DR: the point is to make development much easier by minimizing the time and effort spent on setting up the environment and IDE, and helping to troubleshoot the issues associated with it, thanks to a universal base configuration and reproducible environment, and as a nice bonus, making formatting and clang-tidy checks easier for everyone, which makes using the proposed configuration the optimal and recommended setup option regardless of the OS.

Regarding CI verification, just like with nix flake, kind users will definitely help and update dependencies when necessary :)

@aobolensk
Copy link
Member

Regarding CI verification, just like with nix flake, kind users will definitely help and update dependencies when necessary :)

I don't really agree here, because NixOS is kind of rare use case for people who are already aware of Linux, and they are most likely capable of maintaining their dev environments + NixOS is not really Docker friendly. Here you suggest a go-to system for almost everyone. It means that it should be more or less stable. And to see this stability, I suggest adding Docker based CI.

@allnes
Copy link
Member

allnes commented Oct 22, 2024

@kmichaelk As I understand the statistics on the Visual Studio side of this project and not on VSCode, don't forget, this is a learning project. I agree with Arseniy, alas, without checking CI, docker container seems to me not a stable functionality.
A common problem with this repository is that people don't see what they are changing and mess up their own requests. By creating .vscode we make development more complicated for those who don't know much about VSCode and use it as a notepad. Again, big part of people are not professionals here, but only students, which for some people will be easy, but for the majority will create a problem.
I'm ready to make a survey of what people use and if it turns out that vscode is not dominant, alas it will not be merged.

@0P4RY5H
Copy link
Contributor

0P4RY5H commented Oct 22, 2024

And what's the problem in that case - if VSCode is not the dominant IDE, then these configs will not play any role in principle, and the use of the container will remain the lot of red-eyed people who have enough competencies to fix the environment and deliver a fix for everyone if it suddenly breaks for some reason.

If someone opens a project in VSCode without the clangd extension installed, then the only problem in the configuration, in addition to the CMake setting mentioned by @kmichaelk, will be disabled IntelliSense in the default Microsoft C++ extension, but at this stage it is easier to use the container than to try to configure the environment at all.

And in principle, encouraging (not forcing) people to learn about containerization is much better than suggesting them to poke in the terminal to generate sln and switch to Visual Studio, or even use its built-in CMake support, which is no less inconvenient.

Konstantin-Grudzin added a commit to Konstantin-Grudzin/ppc-2024-autumn that referenced this pull request Oct 27, 2024
PutinVVV pushed a commit to PutinVVV/ppc-2024-autumn that referenced this pull request Nov 20, 2024
PutinVVV pushed a commit to PutinVVV/ppc-2024-autumn that referenced this pull request Nov 21, 2024
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.

8 participants