Conversation
Test Results858 tests 858 ✅ 11m 45s ⏱️ Results for commit 1c40aa8. ♻️ This comment has been updated with latest results. |
…itecture at runtime
bschwedler
left a comment
bschwedler
left a comment
There was a problem hiding this comment.
I like this approach. It is much simpler than having to replace a placeholder value at runtime.
My biggest concern is the implactions of using mv and how it will affect uv. I expect that we will probably be installing uv by default in at least the standard workbench & connect container images.
| {# Normalizes Python installation paths for one or more Python versions with uv | ||
|
|
||
| :param versions: A list of Python versions to install, e.g. ["3.11", "3.12"]. | ||
| #} | ||
| {% macro run_normalize_uv_python_paths(versions) -%} | ||
| {%- if versions is string -%} | ||
| {%- set versions = versions | split(",") | map('trim') -%} | ||
| {%- endif -%} | ||
| {% for version in versions -%} | ||
| {% if loop.first %}RUN {% endif %}mv /opt/python/cpython-{{ version }}-linux-*/ /opt/python/{{ version }}{% if not loop.last %} && \{% endif %} | ||
| {% endfor -%} | ||
| {%- endmacro %} |
There was a problem hiding this comment.
Will using mv will cause problems if/when we start installing uv in the container?
There was a problem hiding this comment.
I think we would modify the macros if we move to managing using uv. I imagine it would break on this as it would look for that matching {python}-{version}-{os}-{arch}-{compiler} pattern. It's possible that fully committing to uv would be the best path, but I wouldn't want to do that in this PR.
| return _get_value_from_env("versionEnvVar", self.versionEnvVar) | ||
|
|
||
| def get_url_by_os(self) -> dict[str, str]: | ||
| def get_url_by_os(self, **kwargs) -> dict[str, str]: |
There was a problem hiding this comment.
Is generalize_architecture the only kwarg that is passed in?
There was a problem hiding this comment.
Yes, but it should go unused in this context I'd think since this would intake an environment variable? Unless you think I should have it do a search and replace on amd64/x86_64 here too. I went back and forth on it. What do you think?
| r"(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?" | ||
| r"(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?" | ||
| ) | ||
| URL_WITH_ENV_VARS_REGEX_PATTERN = re.compile( |
There was a problem hiding this comment.
Could you please add some tests around this to make sure the regex does what we expect?
There was a problem hiding this comment.
Good call, it was wrong before. I fixed it and added tests. I actually ended up removing it from the RelaseStreamResult.downloadUrl parameter too since those should always be valid URLs in the current impl.
2 participants
I took a slightly different approach here. I think it's easiest to use the
$TARGETARCHbuild argument in Dockerfiles since it allows for multiplatform builds on the same file for both emulation and native builds. The only tricky part is when the platforms use a different naming convention, but this can be solved for with some bash expressions.We can continue to clean up this implementation, but I believe this is our shortest path to working builds.