[WIP] feat: path resolver for languages #35
Conversation
| import os | ||
| import logging | ||
|
|
||
|
|
||
| LOG = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def which(cmd, mode=os.F_OK | os.X_OK, path=None): |
There was a problem hiding this comment.
Did you cp this code from Python 3's source? Did you check the license? Did you also check if an existing port is already available?
There was a problem hiding this comment.
Correct. couldnt find a port, but wil check the license.
| Base Class for Runtime Validators | ||
| """ | ||
|
|
||
| import abc |
There was a problem hiding this comment.
Not a big fan of ABCs. Let's be grown ups and let the sub-classes decide which methods to impelement. Obviously if you didn't implement the right methods your subclass code will fail. raising NotImplementedError seems good enough
Thoughts?
There was a problem hiding this comment.
I think abcs are helpful for a contract to be established. In the case of this class, its fairly opinionated on what it does. but if the class goes beyond what it currently does, we can think of moving away from abcs. But other than that, I dont see any harm in this ABC.
| self.artifacts_dir = artifacts_dir | ||
| self.manifest_path = manifest_path | ||
| self.scratch_dir = scratch_dir | ||
| self.runtime = runtime | ||
| self.package_builder = PythonPipDependencyBuilder() | ||
| self.language_exec_path = runtime_path | ||
| self.pip = SubprocessPip(osutils=OSUtils(), python_exe=runtime_path) |
| def __init__(self, language, runtime): | ||
| self.language = language | ||
| self.runtime = runtime | ||
| self.executables = [self.runtime, self.language] |
There was a problem hiding this comment.
This is super limiting. I can have dotnetcore2.0 be the runtime and dotnetcore be the language. But the actual executable is called dotnet
I think you have optimized specifically for Python. We need to think more generally for other languages.
There was a problem hiding this comment.
True it is inspired from python's packages (tox, pipenv). My thinking is that the lambda runtime needs to be prioritized so that we can create arbitrary mapping via symlinks.
|
|
||
| super(PythonPipWorkflow, self).__init__(source_dir, | ||
| artifacts_dir, | ||
| scratch_dir, | ||
| manifest_path, | ||
| runtime_path=self.runtime_path, |
There was a problem hiding this comment.
I understand what you are doing here, but the classes seem to be coupled tightly thru the constructor. Through the constructor you get inputs that are common to every workflow. These inputs are agnostic of how the specific workflow works.
But items like runtime_path are highly dependent on the workflow's implementation. In fact, i can imagine a scenario where a workflow conditionally chooses different runtime_path values when the workflow is actually being run.
Can you explore a different design pattern instead?
In my mind, the Workflows to follow a Plugin design pattern
There was a problem hiding this comment.
Yeah one of the other actions we can explore is runtime validation being done as part of an action, and pass state via callbacks. The state in this case, is the path is still required for another build action. This could be the same with another language too.
|
This PR would be superseded by: #55 (which takes a more general approach) |
|
Closing this PR |
Very much a work in progress PR which will look to find runtime in your path, followed by the language.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.