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.

Sign up for GitHub

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

Jump to bottom

bazel/README.md: add aspell comment #17072

Merged
merged 3 commits into from
Jul 5, 2021
Merged

bazel/README.md: add aspell comment #17072

merged 3 commits into from
Jul 5, 2021

Conversation

daixiang0
Copy link
Member

Signed-off-by: Long Dai [email protected]

Commit Message:

Add aspell comment.

To fix below error:

root:[envoy]$ ./tools/spelling/check_spelling_pedantic.py fix
Traceback (most recent call last):
  File "./tools/spelling/check_spelling_pedantic.py", line 835, in <module>
    rv = execute(target_paths, args.dictionary, args.operation_type == 'fix')
  File "./tools/spelling/check_spelling_pedantic.py", line 735, in execute
    checker.start()
  File "./tools/spelling/check_spelling_pedantic.py", line 184, in start
    universal_newlines=True)
  File "/usr/lib/python3.6/subprocess.py", line 729, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.6/subprocess.py", line 1364, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'aspell': 'aspell'

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@daixiang0
Copy link
Member Author

/cc @phlax

alyssawilk
alyssawilk previously approved these changes Jun 22, 2021
View reviewed changes
@alyssawilk alyssawilk self-assigned this Jun 22, 2021
phlax
phlax requested changes Jun 22, 2021
View reviewed changes
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

@daixiang0 is this the best place to document this ?

bazel/README.md Outdated
@@ -874,7 +874,7 @@ to run clang-format scripts on your workstation directly:

To run the tools directly, you must install the correct version of clang. This
may change over time, check the version of clang in the docker image. You must
also have 'buildifier' installed from the bazel distribution.
also have 'buildifier' installed from the bazel distribution and `aspell`.
Copy link
Member

Choose a reason for hiding this comment

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

im not sure if this is the correct place to put this - given that currently aspell is required for running the python script and isnt activated via bazel - altho this may change after

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not find a good place to put it, maybe a new line for it?

Copy link
Member

Choose a reason for hiding this comment

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

i think that information about aspell should not be in this file - there is no bazel job that depends on it - this file is mostly about setting up a bazel environment - info on linting is not relevant here and is wierdly misplaced

that said - its already like that and there is information about running the spell checker below

perhaps add it there - something like "if you wish to run spellchecking please install aspell"

even that tho doesnt really cover it - eg if you install on debian/ubuntu aspell as so apt install --no-install-recommends aspell it wont work correctly - as you need the en dictionaries that are pulled in as recommends

i think this info - like the information about linting - has no place on this page

Copy link
Member Author

Choose a reason for hiding this comment

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

This section is "Running clang-format without docker", it covers more things indeed, why not rename it as something like "Running format without docker" and comment its dependence like aspell?

Copy link
Member

Choose a reason for hiding this comment

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

rename it as something like "Running format without docker"

my preference would be "Linting your code without docker"

i still think we need to move this section - but agree its beyond the scope of this change

if you want to add information about the aspell dependency - please make it clear that its required for running the check_spelling.py script - not for running the other format tools

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

let us do it step by step :)

alyssawilk
alyssawilk reviewed Jun 22, 2021
View reviewed changes
bazel/README.md Outdated
@@ -874,7 +874,7 @@ to run clang-format scripts on your workstation directly:

To run the tools directly, you must install the correct version of clang. This
may change over time, check the version of clang in the docker image. You must
also have 'buildifier' installed from the bazel distribution.
also have 'buildifier' installed from the bazel distribution and `aspell`.
Copy link
Contributor

Choose a reason for hiding this comment

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

actually in retrospect maybe have the same 's around buildifier and aspell?

/wait

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry I do not get key :(

@daixiang0
feedback
92076ab 
Signed-off-by: Long Dai <[email protected]>
@alyssawilk alyssawilk assigned phlax and unassigned alyssawilk Jun 24, 2021
phlax
phlax reviewed Jun 25, 2021
View reviewed changes
bazel/README.md Outdated Show resolved Hide resolved
phlax
phlax reviewed Jun 25, 2021
View reviewed changes
bazel/README.md Outdated Show resolved Hide resolved
phlax
phlax requested changes Jun 25, 2021
View reviewed changes
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

@daixiang0 a couple of nits then lets land

@daixiang0
feedback
0377175 
Signed-off-by: Long Dai <[email protected]>
@daixiang0
Copy link
Member Author

@phlax done.

@daixiang0
Copy link
Member Author

please review @phlax

phlax
phlax approved these changes Jul 5, 2021
View reviewed changes
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @daixiang0 - with the caveat, as discussed, that you will need the same aspell dictionaries as ci to correctly spellcheck

@phlax phlax merged commit a0ca08b into envoyproxy:main Jul 5, 2021
@daixiang0 daixiang0 deleted the aspell branch July 6, 2021 01:22
baojr added a commit to baojr/envoy that referenced this pull request Jul 7, 2021
@baojr
Merge branch 'main' into baojr/grpc-reverse-bridge-stream
2a09e60 
* main:
  listener: match rebalancer to listener IP family type (envoyproxy#16914)
  jwt_authn: implementation of www-authenticate header (envoyproxy#16216)
  listener: reset the file event in framework instead of listener filter doing itself (envoyproxy#17227)
  Small typo fix (envoyproxy#17247)
  Doc: Clarify request/response attributes are http-only (envoyproxy#17204)
  bazel/README.md: add aspell comment (envoyproxy#17072)
  docs: Fix broken URL links in HTTP upgrades doc (envoyproxy#17225)
  remove the wrong comment on test (envoyproxy#17233)
  upstream: allow clusters to skip waiting on warmup for initialization (envoyproxy#17179)
  JwtAuthn: support completing padding on forward jwt payload header (envoyproxy#16752)
  remove support for v2 UNSUPPORTED_REST_LEGACY (envoyproxy#16968)
  metrics service: fix wrong argument arrange on MetricsServiceSink (envoyproxy#17127)
  deps: update cel-cpp to 0.6.1 (envoyproxy#16293)
  Add ability to filter ConfigDump. (envoyproxy#16774)
  examples: fix Wasm example. (envoyproxy#17218)
  upstream: update host's socket factory when metadata is updated. (envoyproxy#16708)

Signed-off-by: Garrett Bourg <[email protected]>
chrisxrepo pushed a commit to chrisxrepo/envoy that referenced this pull request Jul 8, 2021
@daixiang0 @chrisxrepo
bazel/README.md: add aspell comment (envoyproxy#17072)
e25b326 
Signed-off-by: Long Dai <[email protected]>
Signed-off-by: chris.xin <[email protected]>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
@daixiang0
bazel/README.md: add aspell comment (envoyproxy#17072)
e34a5a8 
Signed-off-by: Long Dai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alyssawilk alyssawilk alyssawilk left review comments

@phlax phlax phlax approved these changes

Assignees

@phlax phlax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daixiang0 @phlax @alyssawilk