-
Notifications
You must be signed in to change notification settings - Fork 382
Move runtime.txt
parsing into base class
#1428
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
base: main
Are you sure you want to change the base?
Conversation
This will allow it to be used for Python (and other languages) in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. A small change in docs/source/changelog.md
is needed.
@@ -1,5 +1,10 @@ | |||
# Changelog | |||
|
|||
## Unreleased breaking changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will require a rebase given that we made a release in August 2025, see https://github.com/jupyterhub/repo2docker/releases/tag/2025.08.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog wasn't updated for that release, I've added to reminder to fix that
#1449
except FileNotFoundError: | ||
return self._runtime | ||
|
||
name = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only support Python and R. I prefer to validate name
against a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, but it breaks the abstraction where subclasses are responsible for calling and validating .runtime
. runtime.txt
is only parsed if the property is accessed, so we can't guarantee any validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There's a discussion somewhere (I can't remember where) about using https://github.com/astrofrog/pypi-timemachine or similar so Python packages can be installed as if
pip
were run in the past, similar to R. If we do this we should define the cutoff date for Python in the same way as for R, which is to useruntime.txt
.Something to consider is we haven't formally defined what
runtime.txt
contains.Our docs state it should be one of:
name-version-yyyy-mm-dd
: (r-<RVERSION>-<YYYY>-<MM>-<DD>
)name-version
: (python-x.y
)repo2docker/docs/source/config_files.rst
Lines 201 to 207 in e795060
Our tests say it can also be:
repo2docker/tests/unit/test_r.py
Line 38 in e795060
This PR handles all three formats. I've made a breaking change and redefined the existing
.runtime
property instead of creating a new one to avoid leaving an unused.runtime
property that may cause confusion in the future.