Skip to content

Vscode bootstrap#406

Merged
mum4k merged 16 commits intoenvoyproxy:masterfrom
oschaaf:vscode-bootstrap
Aug 4, 2020
Merged

Vscode bootstrap#406
mum4k merged 16 commits intoenvoyproxy:masterfrom
oschaaf:vscode-bootstrap

Conversation

@oschaaf
Copy link
Copy Markdown
Member

@oschaaf oschaaf commented Jul 15, 2020

Initial VSCode workspace.

  • Minimal workspace
  • Common tasks pre-setup.
  • Refreshes the compilation database generation script with Envoy's version.
  • Sample debug launch task
  • Python linting / autoformatting configuration

Prerequisite:

oschaaf added 8 commits July 13, 2020 17:59
Also, run gcc-built tests in CI because now we can.

Fixes envoyproxy#382

-----

Prerequisite: envoyproxy/envoy#12057

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added the waiting-for-review A PR waiting for a review. label Jul 15, 2020
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Jul 15, 2020

/cc @pamorgan

@mum4k mum4k requested a review from wjuan-AFK July 15, 2020 14:15
@mum4k
Copy link
Copy Markdown
Collaborator

mum4k commented Jul 15, 2020

@wjuan-AFK please review and assign back to me once done.

oschaaf added 2 commits July 15, 2020 19:26
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Jul 17, 2020

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🙀 failed invoking rebuild of ci/circleci: coverage: 403 Forbidden

🐱

Caused by: a #406 (comment) was created by @oschaaf.

see: more, trace.

Copy link
Copy Markdown
Contributor

@wjuan-AFK wjuan-AFK left a comment

Choose a reason for hiding this comment

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

I think that the tasks portion was working well. I think we might need more documentation indicating that the intellisense in this version only seems to work with the microsoft cpp plugin rather than with clangd, and that you have to run tools/gen_compilation.py and what to do in case vscode doesn't have the pop up. Also, there were those two or three paths that I needed to add to the c_cpp_properties.json. I'm not sure if we should edit the script to include those, or edit the instructions to indicate that those might be desireable.

@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Jul 29, 2020

I think that the tasks portion was working well. I think we might need more documentation indicating that the intellisense in this version only seems to work with the microsoft cpp plugin rather than with clangd, and that you have to run tools/gen_compilation.py and what to do in case vscode doesn't have the pop up. Also, there were those two or three paths that I needed to add to the c_cpp_properties.json. I'm not sure if we should edit the script to include those, or edit the instructions to indicate that those might be desireable.

Shall we punt the docs on intellisense to an issue then? That part is intentionally left that out of here.
Ideally we doc at least the clang language server and its plugin (and potentially other ones, like the ms c++ one) in one go.
For now we could omit this part, and leave it up to the user to configure the IDE correctly on that front;
(though we do offer a way to generate compile_commands.json (which we also need for clang-tidy)).

@wjuan-AFK
Copy link
Copy Markdown
Contributor

Ok. That sounds good then.

I think that the tasks portion was working well. I think we might need more documentation indicating that the intellisense in this version only seems to work with the microsoft cpp plugin rather than with clangd, and that you have to run tools/gen_compilation.py and what to do in case vscode doesn't have the pop up. Also, there were those two or three paths that I needed to add to the c_cpp_properties.json. I'm not sure if we should edit the script to include those, or edit the instructions to indicate that those might be desireable.

Shall we punt the docs on intellisense to an issue then? That part is intentionally left that out of here.
Ideally we doc at least the clang language server and its plugin (and potentially other ones, like the ms c++ one) in one go.
For now we could omit this part, and leave it up to the user to configure the IDE correctly on that front;
(though we do offer a way to generate compile_commands.json (which we also need for clang-tidy)).

@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Jul 31, 2020

Filed #423 to track documentation and/or configuration of plugins.

Copy link
Copy Markdown
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Can we also add some (at least basic) documentation that will indicate how to use these?

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jul 31, 2020
oschaaf added 3 commits July 31, 2020 20:17
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
…vscode-bootstrap

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jul 31, 2020
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Jul 31, 2020

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Jul 31, 2020

Looks like the updated compilation database generation script made us catch a small clang-tidy nit after
rebasing to master:

clang-tidy -extra-arg-before=-xc++ -p=/root/project -quiet /build/tmp/_bazel_bazel/f85b6fb5740e6e8c7efea142eec4b6e8/execroot/nighthawk/include/nighthawk/adaptive_load/scoring_function.h
include/nighthawk/adaptive_load/scoring_function.h:5:10: error: 'envoy/common/pure.h' file not found [clang-diagnostic-error]
#include "envoy/common/pure.h"

Added af5ff2a to resolve. /cc @eric846

mum4k
mum4k previously approved these changes Jul 31, 2020
Copy link
Copy Markdown
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Looks good although arguably I don't know much about Vscode.

Just one question.

@@ -0,0 +1,253 @@
{
// See https://go.microsoft.com/fwlink/?LinkId=733558
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this comment format work in JSON files? I.e. was this tested after the addition of the comment?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes that works. json on steroids ;-)

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jul 31, 2020
@oschaaf oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jul 31, 2020
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Aug 1, 2020

@eric846 because clang-tidy didn't check the adaptive controller interface headers earlier - it only starts doing that with the changes in here - we missed some deps earlier; I added them in the last couple of commits. clang-tidy is happy with the way things are here, it might be good to incorporate those in your outstanding PRs (or wait for this to land and merge master into them afterwards).

@mum4k mum4k merged commit 376deef into envoyproxy:master Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-review A PR waiting for a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants