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.

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

Validate and maybe prune interpreter cache run over run #7225

Merged
merged 5 commits into from Feb 11, 2019
Merged
Diff settings

Always

Just for now

Rename purge method to make side effects more clear

  • Loading branch information...
Chris Livingston
Chris Livingston committed Feb 6, 2019
commit 9362424e257274fb536b4c1e752cc0053616edea
@@ -97,7 +97,6 @@ def select_interpreter_for_targets(self, targets):

tgts_by_compatibilities, total_filter_set = self.partition_targets_by_compatibility(targets)
allowed_interpreters = set(self.setup(filters=total_filter_set))

# Constrain allowed_interpreters based on each target's compatibility requirements.
for compatibility in tgts_by_compatibilities:
compatible_with_target = set(self._matching(allowed_interpreters, compatibility))
@@ -118,9 +117,11 @@ def select_interpreter_for_targets(self, targets):
def _interpreter_from_path(self, path, filters=()):
try:
executable = os.readlink(os.path.join(path, 'python'))
if not os.path.exists(executable):
if os.path.dirname(path) == self._cache_dir:

This comment has been minimized.

Copy link
@jsirois

jsirois Feb 7, 2019

Member

This is always True in production use from my reading of call-sites in this file. How about re-structuring this private function to allow killing this misleading conditional. Maybe re-name to _interpreter_from_relpath and do the path = os.path.joint(self._cache_dir, relpath) here allowing unambiguous removal of the conditional.

self._purge_interpreter(path)
return None
except OSError:
if os.path.dirname(path) == self._cache_dir:
self._purge_interpreter(path)
return None
interpreter = PythonInterpreter.from_binary(executable, include_site_extras=False)
if self._matches(interpreter, filters=filters):
@@ -79,7 +79,7 @@ def execute(self):
if not os.path.exists(interpreter_path_file):
self._create_interpreter_path_file(interpreter_path_file, python_tgts)
else:
if _detect_and_purge_invalid_interpreter(interpreter_path_file):
if self._detect_and_purge_invalid_interpreter(interpreter_path_file):
self._create_interpreter_path_file(interpreter_path_file, python_tgts)

interpreter = self._get_interpreter(interpreter_path_file)
@@ -98,6 +98,17 @@ def _create_interpreter_path_file(self, interpreter_path_file, targets):
def _interpreter_path_file(self, target_set_id):
return os.path.join(self.workdir, target_set_id, 'interpreter.info')

def _detect_and_purge_invalid_interpreter(self, interpreter_path_file):
with open(interpreter_path_file, 'r') as infile:

This comment has been minimized.

Copy link
@jsirois

jsirois Feb 7, 2019

Member

Instead of re-creating the file reading logic, how about something like:

interpreter = self._get_interpreter(interpreter_path_file)
if not os.path.exists(os.readlink(interpreter.binary)):
  ...
lines = infile.readlines()
binary = lines[0].strip()
if not os.path.exists(binary):
self.context.log.info('Stale interpreter reference detected: {}, removing reference and '
'selecting a new interpreter.'.format(binary))
os.remove(interpreter_path_file)
return True
return False

@staticmethod
def _get_interpreter(interpreter_path_file):
with open(interpreter_path_file, 'r') as infile:
@@ -108,15 +119,3 @@ def _get_interpreter(interpreter_path_file):
dist_name, dist_version, location = line.strip().split('\t')
interpreter = interpreter.with_extra(dist_name, dist_version, location)
return interpreter

@staticmethod
def _detect_and_purge_invalid_interpreter(interpreter_path_file):
with open(interpreter_path_file, 'r') as infile:
lines = infile.readlines()
binary = lines[0].strip()
if not os.path.exists(binary):
self.context.log.info('Stale interpreter reference detected, removing reference and '
'selecting a new interpreter.')
os.remove(interpreter_path_file)
return True
return False
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.