Inferring source distribution and python version #428
Conversation
|
This is awesome, thanks for adding it! We've been wanting this for a while :) |
pkg/rebuild/pypi/infer.go
Outdated
| if strings.Contains(r, "python_version") { | ||
| r = strings.Split(r, ";")[0] | ||
| } |
There was a problem hiding this comment.
Should we just strip all quoted_markers as described in the dependency spec?
If I understand the grammar correctly, we could maybe just always split on ";" regardless of the "python_version" being present?
Otherwise, maybe we should match on the full env_var?
| return nil, fs.ErrNotExist | ||
| } | ||
|
|
||
| func inferRequirements(name, version string, zr interface{}) ([]string, error) { |
There was a problem hiding this comment.
If possible, it would be nice to avoid interface{} types in the function signature and as local variables. Without a specific interface type, it becomes harder to reason about what the code is going to do with this object.
In this case, I think the function immediately returns with an empty reqs when a tar.Reader is provided? If so, then maybe this could stay specific to just *zip.Reader, and the caller can just use an empty reqs when processing tar files?
There was a problem hiding this comment.
Or maybe this could accept a function that just reads files, and it could be independent of zip or tar dependencies?
There was a problem hiding this comment.
I'm still interested to see if we can find a way to avoid this interface{} type. Could we use just a regular reader type?
pkg/rebuild/pypi/strategy.go
Outdated
| if ! command -v curl &> /dev/null; then | ||
| apk add clang curl build-base patch zip zlib-dev libffi-dev linux-headers readline-dev openssl openssl-dev sqlite-dev bzip2-dev xz-dev | ||
| fi | ||
| curl https://raw.githubusercontent.com/pyenv/pyenv-installer/master/bin/pyenv-installer | bash |
There was a problem hiding this comment.
Do you think the pyenv-installer should be pinned?
I'd maybe lean towards yes but I'm open to suggestions there.
There was a problem hiding this comment.
If we don't infer the version of python it may be useful to pin it. But I don't have a strong opinion here, for what I saw so far the python version has a limited impact on the build. However, it may be useful from an attestation point of view.
pkg/rebuild/pypi/strategy.go
Outdated
| Runs: textwrap.Dedent(` | ||
| if [ ! -d "/root/.pyenv" ]; then | ||
| if ! command -v curl &> /dev/null; then | ||
| apk add clang curl build-base patch zip zlib-dev libffi-dev linux-headers readline-dev openssl openssl-dev sqlite-dev bzip2-dev xz-dev |
There was a problem hiding this comment.
This is quite a big set of dependencies, are these all required for installing python? I guess I shouldn't be surprised, maybe we should pin version numbers here?
There was a problem hiding this comment.
Those dependencies were only for using pyenv. Even if we end up by using it, we can probably find a smarter way to deal with this!
| return nil, fs.ErrNotExist | ||
| } | ||
|
|
||
| func inferRequirements(name, version string, zr interface{}) ([]string, error) { |
There was a problem hiding this comment.
I'm still interested to see if we can find a way to avoid this interface{} type. Could we use just a regular reader type?
Signed-off-by: Giacomo Benedetti <[email protected]>
For source distribution:
For Python version:
This PR requires the
Executorlogic proposed in PR #427In case that PR isn't accepted I can decouple this dependency!