Skip to content

Fix problems with bazel query#5821

Closed
robertpanzer wants to merge 2 commits intoenvoyproxy:masterfrom
robertpanzer:fix-bazel-info
Closed

Fix problems with bazel query#5821
robertpanzer wants to merge 2 commits intoenvoyproxy:masterfrom
robertpanzer:fix-bazel-info

Conversation

@robertpanzer
Copy link
Copy Markdown

Description:
At the moment it is impossible for me to import the Envoy repository into VS Code using the "official" Bazel plugin.
This is because it calls bazel query which fails on problems with the BUILD files in the api folder:

$ /usr/local/bin/bazel query ... --output=package --output=package
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/robert/dev/envoy/tools/bazel.rc
INFO: Invocation ID: 5a9ca157-f351-4424-ace6-524cb935659a
ERROR: error loading package 'api/envoy/config/metrics/v2': Unable to load file '//bazel:api_build_system.bzl': file doesn't exist

This PR fixes this by changing the load path.

Additionally I renamed the file examples/grpc-bridge/script/build to examples/grpc-bridge/script/build.sh as bazel query identifies this as a Bazel build file on OS X due to the case insensitive file system.

Risk Level:
If the build passes it should be fine.

Testing:
Test that the API still works.

Docs Changes:
None

Release Notes:
None
[Optional Fixes #Issue]
[Optional Deprecated:]

@htuch htuch self-assigned this Feb 4, 2019
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

This seems legit, although strictly not needed for normal use, since api/ is only ever seen as a local repository remount. I assume the vscode plugin isn't smart enough to grok that api/ shouldn't be treated as part of the outer repo.

BTW you also need to reflect the build rename for the gRPC example in https://www.envoyproxy.io/docs/envoy/latest/start/sandboxes/grpc_bridge

@robertpanzer
Copy link
Copy Markdown
Author

Thanks for the super fast feedback! :)
I updated the document, and also moved the script bootstrap to bootstrap.sh.
I think it makes sense to have these 2 shell scripts both have the extension .sh.

@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 4, 2019

Please merge master to pick up #5827.

Renamed examples/grpc-bridge/script/build to avoid problems on OS X with case insensitive filesystems

Signed-off-by: Robert Panzer <robert.panzer.pb@googlemail.com>
Signed-off-by: Robert Panzer <robert.panzer.pb@googlemail.com>
@robertpanzer
Copy link
Copy Markdown
Author

Thanks! I rebased and force pushed.

However I got this error from the recommit checks:

  Checking format for test/common/secret/sds_api_test.cc - ERROR: From ./test/common/secret/sds_api_test.cc
ERROR: clang-format check failed for file: ./test/common/secret/sds_api_test.cc
ERROR: check format failed. run 'tools/check_format.py fix'

I don't know if this only happens when pushing from OSX or if I have a wrong version of clang-format.
So I ignored it for now as this should definitely be unrelated to this PR.

@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 4, 2019

@robertpanzer re: your local error, sometimes you get this if you clang-format is a different version to the one used in CI, so that might be a place to look. I don't personally use the precommit hooks.

@htuch htuch added the waiting label Feb 8, 2019
@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 8, 2019

@robertpanzer can you merge master?

Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

This break data-plane-api to build independently with bazel, should we just put a set of directories to .bazelignore and that might just solve your problem?

@mergeconflict mergeconflict mentioned this pull request Feb 8, 2019
@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 10, 2019

Closing this out in favor of #5886 which just merged

@htuch htuch closed this Feb 10, 2019
@robertpanzer
Copy link
Copy Markdown
Author

@htuch Sorry I wasn't able to catch up as I was traveling over the weekend.
I just checked the latest master and the problem I reported still exists.
But maybe it's just me.

@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 11, 2019

@robertpanzer @mergeconflict also uses vscode and was the author of #5886. Maybe you can sync on Slack to discuss the workflow?

@robertpanzer
Copy link
Copy Markdown
Author

I created #5909 to track the problem that this PR tried to solve.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants