Coalesce directory management to Envoy::TestEnvironment #9721

wrowe · 2020-01-17T17:55:32Z

Description:

Drop std::[*]filesystem logic
Implement/tests for new Envoy::Filesystem::splitFileName
Remove unneeded TestEnvironment::createParentPath
Move/rename Envoy::TestUtility::createDirectory to Envoy::TestEnvironment::createPath
Move/rename Envoy::TestUtility::removeDirectory to Envoy::TestEnvironment::removePath
Move Envoy::TestUtility::renameFile to Envoy::TestEnvironment
Move Envoy::TestUtility::createSymlink to Envoy::TestEnvironment
Move AtomicFileUpdater from test_common utility to environment
Simplify Envoy::Filesystem::Watcher implementations using Filesystem
Clean up watcher_impl flavors for splitFileName API
Implement/new tests for Envoy::Filesystem::illegalPath for win32
Implement watcher_impl for win32 [Initial development by Sam Smith]
Ensure top level test tempdir exists when requested

Risk Level: low
Testing: local on linux gcc, windows msvc
Docs Changes: n/a
Release Notes: n/a

tools/check_format.py

- Drop std::[*]filesystem logic - Implement/tests for new Envoy::Filesystem::splitFileName - Remove unneeded TestEnvironment::createParentPath - Move/rename Envoy::TestUtility::createDirectory to Envoy::TestEnvironment::createPath - Move/rename Envoy::TestUtility::removeDirectory to Envoy::TestEnvironment::removePath - Move Envoy::TestUtility::renameFile to Envoy::TestEnvironment - Move Envoy::TestUtility::createSymlink to Envoy::TestEnvironment - Move AtomicFileUpdater from test_common utility to environment - Simplify Envoy::Filesystem::Watcher implementations using Filesystem - Clean up watcher_impl flavors for splitFileName API - Implement/new tests for Envoy::Filesystem::illegalPath for win32 - Implement watcher_impl for win32 [Initial development by Sam Smith] - Ensure top level test tempdir exists when requested Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Avert making envoy tweaks within 'clang-format off' blocks Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

mattklein123

Overall LGTM, thanks for slogging through this. Some questions/comments. Note that I just did a very fast skim of the win32 watcher code so I would make sure to have other folks review that part.

/wait

include/envoy/filesystem/filesystem.h

source/common/filesystem/inotify/watcher_impl.h

source/common/filesystem/win32/filesystem_impl.cc

source/common/filesystem/win32/watcher_impl.cc

source/common/filesystem/win32/watcher_impl.h

test/common/filesystem/filesystem_impl_test.cc

test/test_common/environment.cc

tools/check_format.py

test/test_common/utility.h

- Streamline the clang-format sensitivity for Envoy check/fix - Rename splitFileName to splitPathFromFilename for clarity and change to absl::string_views for args and std::pair results - Expose Api to watcher_impl for Filesystem - All addWatch() implementations now take absl::string_view name args - Clean up member variable naming for win32 watcher_impl - Prefer RELEASE_ASSERT to throw for anticipated normal behavior Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

Don't blow up in the DirectoryIterator for a broken link. Treat broken symlinks as 'Regular' files to be unlinked. Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

mattklein123

LGTM modulo some nits. Thank you!

/wait

include/envoy/filesystem/filesystem.h

mattklein123 · 2020-01-22T23:27:39Z

source/common/filesystem/posix/directory_iterator_impl.cc

+      if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode))
+        return FileType::Regular;


braces around all if statements please, here and below. This should be caught by clang-tidy but clang-tidy is currently broken. @lizan @derekargueta any ETA on fixing that?

Ack, reviewed and fixed throughout. Leaving unresolved for the open question of clang-tidy.

source/common/filesystem/posix/directory_iterator_impl.cc

source/common/filesystem/posix/filesystem_impl.cc

source/common/filesystem/win32/filesystem_impl.cc

test/common/filesystem/filesystem_impl_test.cc

test/test_common/environment.cc

tools/check_format.py

jmarantz

check_format.py looks great generally; nice improvement. Just a minor suggestion.

tools/check_format.py

Note: kqueue remains untested locally Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

- describe changes to directory_iterator_impl - use braces on all if/for statements - document source of windows invalid character table - add more description to AtomicFileUpdater class - capitalize test names in filesystem_impl_test Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

- clang-format off blocks must be closed with clang-format on - clang-format may not be nested Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

mattklein123

Awesome, LGTM other than small remaining comments and master merge.

/wait

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

mattklein123

Thanks LGTM with small remaining nit/cleanup.

/wait

source/common/filesystem/inotify/watcher_impl.cc

Resolves test failing due to no file to substitute within Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

wrowe · 2020-01-23T22:14:41Z

nit: this error-message syntax is repeated also in the def reportError code in the call-site. I think a small refactoring can be done where you pass in a reportError function to evaluateLines from its call-site.
This will require some minor function refactoring as the reportError function captures the line-number from context, and you'd need to factor out one that took the line-number as an argument. But I think it will clean this up a little. Not that it matters a huge amount but it would also enable multiple clang-format errors to be reported; the current structure where you return the error message will only show the last error.

Hi @jmarantz - thanks for your observations and suggestions.

One concern, if we wanted to show >1 error, we would want to constrain this to some sane number of errors (1..100?) I think we are mostly satisfied with the single error, although it can take more iterations to correct all the defects (provided one isn't using check_format.py fix).

Otherwise, @sunjayBhatia and I reviewed your proposal and suspect it would be easier, if you like, to have you directly modify the branch with your proposal as you envision it (or, we can merge master and start a new effort.) Please feel free to @ include us on any re-tooling, since we are frequent users of these facilities :)

jmarantz · 2020-01-23T22:19:19Z

I wouldn't expect a large # of errors but if there were more than one and you only showed one, I'd want the first one and not the last :)

wrowe · 2020-01-23T22:23:44Z

I wouldn't expect a large # of errors but if there were more than one and you only showed one, I'd want the first one and not the last :)

We've updated the code since the first draft to report on the first issue encountered.

Initially we didn't believe the test framework required this, but those tests named with the convention FooTest/PROTO will require FooTest to first be created before PROTO. Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

wrowe · 2020-01-24T21:55:15Z

I believe the requested changes are now addressed, please clarify if we've missed anything.

Resolves TapIntegrationTest failure Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

jmarantz · 2020-01-24T23:08:52Z

tools/check_format.py

+  for line_number, line in enumerate(readLines(path)):
+    if line.find("// clang-format off") != -1:
+      if not format_flag and error_message is None:
+        error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format nested off")


as suggested earlier, can you pass reportError as a lambda into evaluateLines so you don't have to repeat the error-message formatting logic here & below? You can still show only one clang-format nesting error if you want by keeping a local bool.

I'm going to go ahead and merge this so we don't keep having merge conflicts. @wrowe can you do a follow up to address @jmarantz comments on this file? Thank you.

mattklein123 · 2020-01-26T18:11:04Z

/azp run envoy-linux

azure-pipelines · 2020-01-26T18:11:12Z

Azure Pipelines successfully started running 1 pipeline(s).

mattklein123

Thanks!

wrowe requested review from alyssawilk, lizan and mattklein123 as code owners January 17, 2020 17:55

mattklein123 self-assigned this Jan 17, 2020

wrowe commented Jan 17, 2020

View reviewed changes

tools/check_format.py Outdated Show resolved Hide resolved

wrowe and others added 2 commits January 21, 2020 11:37

Correct envoy-format warnings

a2c4599

Avert making envoy tweaks within 'clang-format off' blocks Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

mattklein123 requested changes Jan 21, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 21, 2020

wrowe and others added 2 commits January 22, 2020 17:28

repokitteh-read-only bot removed the waiting label Jan 22, 2020

mattklein123 requested changes Jan 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 22, 2020

jmarantz reviewed Jan 23, 2020

View reviewed changes

tools/check_format.py Outdated Show resolved Hide resolved

wrowe and others added 3 commits January 23, 2020 12:45

Fix win32 and kqueue watcher implementation typos

dea7313

Note: kqueue remains untested locally Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

repokitteh-read-only bot removed the waiting label Jan 23, 2020

mattklein123 requested changes Jan 23, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 23, 2020

wrowe and others added 3 commits January 23, 2020 14:45

Change splitPathFromFilename to return a struct with named members

609180e

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

Merge branch 'master' into fix-filesystem-usage

b019384

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

Make the spell checker happy

3eab6da

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

repokitteh-read-only bot removed the waiting label Jan 23, 2020

sunjayBhatia and others added 2 commits January 23, 2020 15:37

Catch more ifs without braces

d2ded9d

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

More mac typos/compilation fixes

6d34908

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

mattklein123 requested changes Jan 23, 2020

View reviewed changes

source/common/filesystem/inotify/watcher_impl.cc Outdated Show resolved Hide resolved

source/common/filesystem/inotify/watcher_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jan 23, 2020

Clean up failing test

e9e095c

Resolves test failing due to no file to substitute within Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

More requested reviews in watcher_impl

5f233df

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

repokitteh-read-only bot removed the waiting label Jan 23, 2020

mattklein123 added the waiting label Jan 24, 2020

Introduce mkdir -p recursive behavior

e340a78

Initially we didn't believe the test framework required this, but those tests named with the convention FooTest/PROTO will require FooTest to first be created before PROTO. Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

repokitteh-read-only bot removed the waiting label Jan 24, 2020

wrowe requested a review from mattklein123 January 24, 2020 21:53

Don't invoke createPath with a trailing slash

ed7c38e

Resolves TapIntegrationTest failure Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

jmarantz reviewed Jan 24, 2020

View reviewed changes

mattklein123 approved these changes Jan 26, 2020

View reviewed changes

mattklein123 merged commit 1dc6e43 into envoyproxy:master Jan 26, 2020

wrowe deleted the fix-filesystem-usage branch January 27, 2020 23:26

sunjayBhatia mentioned this pull request Feb 28, 2020

NT support #129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coalesce directory management to Envoy::TestEnvironment #9721

Coalesce directory management to Envoy::TestEnvironment #9721

wrowe commented Jan 17, 2020

mattklein123 left a comment

mattklein123 left a comment

mattklein123 Jan 22, 2020

wrowe Jan 23, 2020

jmarantz left a comment

mattklein123 left a comment

mattklein123 left a comment

wrowe commented Jan 23, 2020

jmarantz commented Jan 23, 2020

wrowe commented Jan 23, 2020

wrowe commented Jan 24, 2020

jmarantz Jan 24, 2020

mattklein123 Jan 26, 2020

mattklein123 commented Jan 26, 2020

azure-pipelines bot commented Jan 26, 2020

mattklein123 left a comment

		if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode))
		return FileType::Regular;

Coalesce directory management to Envoy::TestEnvironment #9721

Coalesce directory management to Envoy::TestEnvironment #9721

Conversation

wrowe commented Jan 17, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Jan 22, 2020

Choose a reason for hiding this comment

wrowe Jan 23, 2020

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

wrowe commented Jan 23, 2020

jmarantz commented Jan 23, 2020

wrowe commented Jan 23, 2020

wrowe commented Jan 24, 2020

jmarantz Jan 24, 2020

Choose a reason for hiding this comment

mattklein123 Jan 26, 2020

Choose a reason for hiding this comment

mattklein123 commented Jan 26, 2020

azure-pipelines bot commented Jan 26, 2020

mattklein123 left a comment

Choose a reason for hiding this comment