Skip to content

Commit

Permalink
Add configuration error exit code to client
Browse files Browse the repository at this point in the history 
Summary: Useful to be able to distinguish a failure exit code as being due to a problem with pyre configuration contents

Reviewed By: grievejia

Differential Revision: D23716579

fbshipit-source-id: 2056af4593074bad023b56b893762054e7044a6a
  • Loading branch information
@shannonzhu @facebook-github-bot
shannonzhu authored and facebook-github-bot committed Sep 16, 2020
1 parent 51dd2e9 commit 6798c63
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 143 deletions.
1 change: 1 addition & 0 deletions client/commands/command.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class ExitCode(enum.IntEnum):
BUCK_ERROR = 3
SERVER_NOT_FOUND = 4
INCONSISTENT_SERVER = 5
CONFIGURATION_ERROR = 6
# If the process exited due to a signal, this will be the negative signal number.
SIGSEGV = -signal.SIGSEGV

Expand Down
238 changes: 112 additions & 126 deletions client/configuration.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def consume(

value = self._configuration.pop(key, default)
if raise_on_override and current and value:
raise EnvironmentException(
raise InvalidConfiguration(
f"Configuration file may not override `{key}` field."
)
if current:
Expand Down Expand Up @@ -316,141 +316,127 @@ def from_arguments(
)

def _validate(self) -> None:
try:
def is_list_of_strings(list):
if len(list) == 0:
return True
return not isinstance(list, str) and all(
isinstance(element, str) for element in list
)

def is_list_of_strings(list):
if len(list) == 0:
return True
return not isinstance(list, str) and all(
isinstance(element, str) for element in list
)
if not is_list_of_strings(self.source_directories) or not is_list_of_strings(
self.targets
):
raise InvalidConfiguration(
"`target` and `source_directories` fields must be lists of " "strings."
)

if not is_list_of_strings(
self.source_directories
) or not is_list_of_strings(self.targets):
raise InvalidConfiguration(
"`target` and `source_directories` fields must be lists of "
"strings."
)
if not is_list_of_strings(self.ignore_all_errors):
raise InvalidConfiguration(
"`ignore_all_errors` field must be a list of strings."
)

if not is_list_of_strings(self.ignore_all_errors):
raise InvalidConfiguration(
"`ignore_all_errors` field must be a list of strings."
)
if not is_list_of_strings(self.ignore_infer):
raise InvalidConfiguration(
"`ignore_infer` field must be a list of strings."
)

if not is_list_of_strings(self.ignore_infer):
raise InvalidConfiguration(
"`ignore_infer` field must be a list of strings."
)
if not is_list_of_strings(self.extensions):
raise InvalidConfiguration("`extensions` field must be a list of strings.")
if not all(
extension.startswith(".") or not extension for extension in self.extensions
):
raise InvalidConfiguration(
"`extensions` must only contain strings formatted as `.EXT`"
)

if not is_list_of_strings(self.extensions):
raise InvalidConfiguration(
"`extensions` field must be a list of strings."
)
if not all(
extension.startswith(".") or not extension
for extension in self.extensions
):
raise InvalidConfiguration(
"`extensions` must only contain strings formatted as `.EXT`"
)
if not os.path.exists(self.binary):
raise InvalidConfiguration(f"Binary at `{self.binary}` does not exist.")

if not os.path.exists(self.binary):
raise InvalidConfiguration(f"Binary at `{self.binary}` does not exist.")
if self.number_of_workers < 1:
raise InvalidConfiguration("Number of workers must be greater than 0.")

if self.number_of_workers < 1:
raise InvalidConfiguration("Number of workers must be greater than 0.")
# Validate typeshed path and sub-elements.
assert_readable_directory_in_configuration(self.typeshed, field_name="typeshed")

# Validate typeshed path and sub-elements.
assert_readable_directory_in_configuration(
self.typeshed, field_name="typeshed"
# A courtesy warning since we have changed default behaviour.
if self._typeshed_has_obsolete_value():
LOG.warning(
f"It appears that `{self.typeshed}` points at a `stdlib` "
"directory. Please note that the `typeshed` configuration must "
"point to the root of the `typeshed` directory."
)

# A courtesy warning since we have changed default behaviour.
if self._typeshed_has_obsolete_value():
LOG.warning(
f"It appears that `{self.typeshed}` points at a `stdlib` "
"directory. Please note that the `typeshed` configuration must "
"point to the root of the `typeshed` directory."
)

expanded_ignore_paths = []
for path in self.ignore_all_errors:
expanded = glob.glob(path)
if not expanded:
expanded_ignore_paths.append(path)
else:
expanded_ignore_paths += expanded
self.ignore_all_errors = expanded_ignore_paths
expanded_ignore_paths = []
for path in self.ignore_all_errors:
expanded = glob.glob(path)
if not expanded:
expanded_ignore_paths.append(path)
else:
expanded_ignore_paths += expanded
self.ignore_all_errors = expanded_ignore_paths

non_existent_ignore_paths = [
path for path in self.ignore_all_errors if not os.path.exists(path)
non_existent_ignore_paths = [
path for path in self.ignore_all_errors if not os.path.exists(path)
]
if non_existent_ignore_paths:
LOG.warning(
"Nonexistent paths passed in to `ignore_all_errors`: "
f"`{non_existent_ignore_paths}`"
)
self.ignore_all_errors = [
path
for path in self.ignore_all_errors
if path not in non_existent_ignore_paths
]
if non_existent_ignore_paths:
LOG.warning(
"Nonexistent paths passed in to `ignore_all_errors`: "
f"`{non_existent_ignore_paths}`"
)
self.ignore_all_errors = [
path
for path in self.ignore_all_errors
if path not in non_existent_ignore_paths
]

non_existent_infer_paths = [
path for path in self.ignore_infer if not os.path.exists(path)

non_existent_infer_paths = [
path for path in self.ignore_infer if not os.path.exists(path)
]
if non_existent_infer_paths:
LOG.warning(
"Nonexistent paths passed in to `ignore_infer`: "
f"`{non_existent_infer_paths}`"
)
self.ignore_infer = [
path
for path in self.ignore_infer
if path not in non_existent_infer_paths
]
if non_existent_infer_paths:
LOG.warning(
"Nonexistent paths passed in to `ignore_infer`: "
f"`{non_existent_infer_paths}`"
)
self.ignore_infer = [
path
for path in self.ignore_infer
if path not in non_existent_infer_paths
]

for typeshed_subdirectory_name in ["stdlib", "third_party"]:
typeshed_subdirectory = os.path.join(
self.typeshed, typeshed_subdirectory_name
)
assert_readable_directory_in_configuration(typeshed_subdirectory)
for typeshed_version_directory_name in os.listdir(
typeshed_subdirectory
):
if not typeshed_version_directory_name[0].isdigit():
raise InvalidConfiguration(
"Directories inside `typeshed` must only contain "
"second-level subdirectories starting with "
"a version number."
)
typeshed_version_directory = os.path.join(
typeshed_subdirectory, typeshed_version_directory_name
)
assert_readable_directory_in_configuration(
typeshed_version_directory
)

# Validate elements of the search path.
for element in self._search_path:
assert_readable_directory_in_configuration(
element.path(), field_name="search_path"
for typeshed_subdirectory_name in ["stdlib", "third_party"]:
typeshed_subdirectory = os.path.join(
self.typeshed, typeshed_subdirectory_name
)
assert_readable_directory_in_configuration(typeshed_subdirectory)
for typeshed_version_directory_name in os.listdir(typeshed_subdirectory):
if not typeshed_version_directory_name[0].isdigit():
raise InvalidConfiguration(
"Directories inside `typeshed` must only contain "
"second-level subdirectories starting with "
"a version number."
)
typeshed_version_directory = os.path.join(
typeshed_subdirectory, typeshed_version_directory_name
)
assert_readable_directory_in_configuration(typeshed_version_directory)

if not is_list_of_strings(self.other_critical_files):
raise InvalidConfiguration(
"`critical_files` field must be a list of strings."
)
# Validate elements of the search path.
for element in self._search_path:
assert_readable_directory_in_configuration(
element.path(), field_name="search_path"
)

if not is_list_of_strings(self.do_not_ignore_errors_in):
raise InvalidConfiguration(
"`do_not_ignore_errors_in` field must be a list of strings."
)
for directory_name in self.do_not_ignore_errors_in:
assert_readable_directory_in_configuration(directory_name)
except InvalidConfiguration as error:
raise EnvironmentException(str(error))
if not is_list_of_strings(self.other_critical_files):
raise InvalidConfiguration(
"`critical_files` field must be a list of strings."
)

if not is_list_of_strings(self.do_not_ignore_errors_in):
raise InvalidConfiguration(
"`do_not_ignore_errors_in` field must be a list of strings."
)
for directory_name in self.do_not_ignore_errors_in:
assert_readable_directory_in_configuration(directory_name)

@property
def version_hash(self) -> str:
Expand Down Expand Up @@ -529,7 +515,7 @@ def _check_nested_configurations(self, local_root: str) -> None:
for ignore_element in parent_local_configuration.ignore_all_errors:
if Path(ignore_element).resolve() in paths_to_ignore:
excluded_from_parent = True
except EnvironmentException as error:
except InvalidConfiguration as error:
parent_error = error

if not excluded_from_parent:
Expand All @@ -541,27 +527,27 @@ def _check_nested_configurations(self, local_root: str) -> None:
"into a single configuration, or split the parent configuration to "
"avoid inconsistent errors."
)
raise EnvironmentException(error_message)
raise InvalidConfiguration(error_message)
elif parent_error:
raise EnvironmentException(parent_error)
raise InvalidConfiguration(str(parent_error))

def _check_read_local_configuration(self, path: str) -> None:
if not os.path.exists(path):
raise EnvironmentException(
raise InvalidConfiguration(
f"Local configuration path `{path}` does not exist."
)

local_configuration = os.path.join(path, LOCAL_CONFIGURATION_FILE)
if not os.path.exists(local_configuration):
raise EnvironmentException(
raise InvalidConfiguration(
f"Local configuration directory `{path}` does not contain "
f"a `{LOCAL_CONFIGURATION_FILE}` file."
)

self._check_nested_configurations(path)
self._read(local_configuration)
if not self.source_directories and not self.targets:
raise EnvironmentException(
raise InvalidConfiguration(
f"Local configuration `{local_configuration}` does not specify "
" any sources to type check."
)
Expand Down Expand Up @@ -719,7 +705,7 @@ def _read(self, path: str) -> None:
# We error elsewhere if there weren't enough parameters passed into pyre.
self._expand_relative_paths(configuration_directory)
except json.JSONDecodeError as error:
raise EnvironmentException(
raise InvalidConfiguration(
"Configuration file at `{}` is invalid: {}.".format(path, str(error))
)

Expand Down
16 changes: 10 additions & 6 deletions client/pyre.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,11 @@ def run_pyre(arguments: argparse.Namespace) -> ExitCode:
raise FailedOutsideLocalConfigurationException(
exit_code, command, str(error)
)
except (buck.BuckException, EnvironmentException) as error:
except (
buck.BuckException,
configuration_module.InvalidConfiguration,
EnvironmentException,
) as error:
if arguments.command == commands.Persistent.from_arguments:
try:
commands.Persistent.run_null_server(timeout=3600 * 12)
Expand All @@ -156,11 +160,11 @@ def run_pyre(arguments: argparse.Namespace) -> ExitCode:
exit_code = ExitCode.SUCCESS
else:
client_exception_message = str(error)
exit_code = (
ExitCode.BUCK_ERROR
if isinstance(error, buck.BuckException)
else ExitCode.FAILURE
)
exit_code = ExitCode.FAILURE
if isinstance(error, buck.BuckException):
exit_code = ExitCode.BUCK_ERROR
elif isinstance(error, configuration_module.InvalidConfiguration):
exit_code = ExitCode.CONFIGURATION_ERROR
except commands.ClientException as error:
client_exception_message = str(error)
exit_code = ExitCode.FAILURE
Expand Down
Loading

0 comments on commit 6798c63

Please sign in to comment.