Allow custom overrites for package resolving and optional sys.path support #601
Conversation
|
Matthias Bartelt seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Would it be possible for you to add two options:
- Make respecting sys.path opt in
- Add additional overrides
Example: if codegen is installed a virtual environment, we wouldn't want to make the user set PYTHONPATH/install it into the project environment. We'd also not want to accidentally include code from their virtual enviornment
tests/unit/codegen/sdk/python/import_resolution/test_import_resolution.py
Outdated
Show resolved
Hide resolved
spezifant
changed the title
| # =====[ Check if we are importing an entire file with custom resolve path or sys.path enabled ]===== | ||
| if len(self.ctx.config.py_resolve_overrides) > 0 or self.ctx.config.py_resolve_syspath: | ||
| # Handle resolve overrides first if both is set | ||
| resolve_paths: list[str] = self.ctx.config.py_resolve_overrides + (sys.path if self.ctx.config.py_resolve_syspath else []) |
There was a problem hiding this comment.
Can we refactor this to it's own function?
There was a problem hiding this comment.
I think its fine as is?
There was a problem hiding this comment.
Both would be perfectly fine for me.
| @@ -107,6 +108,13 @@ def resolve_import(self, base_path: str | None = None, *, add_module_name: str | | |||
| if file := self.ctx.get_file(filepath): | |||
There was a problem hiding this comment.
If we do have a sys.path/override configured, can we make sure to use that path to resolve imports in case of conflicts? Like if we have a/b.py and b.py and a path of ["a", ""], we should resolve the import from a if sys path/overrides are enabled.
There was a problem hiding this comment.
Changed the code order and added a dedicated test.
| # =====[ Check if we are importing an entire file with custom resolve path or sys.path enabled ]===== | ||
| if len(self.ctx.config.py_resolve_overrides) > 0 or self.ctx.config.py_resolve_syspath: | ||
| # Handle resolve overrides first if both is set | ||
| resolve_paths: list[str] = self.ctx.config.py_resolve_overrides + (sys.path if self.ctx.config.py_resolve_syspath else []) |
There was a problem hiding this comment.
I think its fine as is?
christinewangcw
temporarily deployed
to
testing
GitHub Actions
Inactive
Motivation
Certain build systems or configurations have Python packages located in a non-default path. Those are not found by Codegen per default.
Content
Codegen now considers the
PYTHONPATHenvironment variable when resolving packages.Testing
Added unit tests which resolve packages only when the
PYTHONPATHis set in a correct way.Please check the following before marking your PR as ready for review