mrtrix3.run: Improved shebang parsing #2958
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Took a quick look on MSYS2. The @daljit46 Do you want to make a case against the pre-existing behaviour where we ensure that executed Python scripts use the same interpreter as that currently executing, potentially overruling the system config? Otherwise I'll merge this. |
|
I don't have strong objections against this PR.
Yes, this is the expectation I have when running a script. I think it's the simplest solution and the most in line with Python's idea of venvs. |
|
In responding on #2261, I realised there may be an outstanding problem with this kind of solution. One of the approaches that had been proposed earlier for controlling the Python module search environment was to have executable files where the shebang points to a softlink to Python. That softlink would reside within the MRtrix3 build / installation directory, and just point to a suitable Python version. However by invoking via that softlink, the MRtrix3 API would intrinsically be included within Python's Say hypothetically that a third party software uses a similar approach, but instead of the shebang pointing to a softlink, it instead points to a Python in eg. a custom Conda environment, which contains custom libraries / library versions requisite for that command. Say for instance: With the code in this PR, that would result in use of the current interpreter, which may cause problems for that command. For the FSL I have installed locally, this isn't the case, they instead use eg.: , which would be fine. But I don't know if this is what FSL has always done, or whether any other third party piece of software could hard-code a custom Python into their shebang. There's a possible alternative strategy that comes to mind:
I don't recall whether there have been any historical edge cases where this would cause problems. But some of them may have had to do with malformed shebangs, which may have been resolved upstream in third-party softwares. I'll let this simmer for a bit, but my current instinct is that this would be a better solution. |
In doing #2957 I noticed an issue whereby whenever I manually invoked my compiled-from-source Python 3.12, any Python scripts that themselves invoke Python scripts would revert to Python 3.10 that was in my
PATH. This is contrary to previously constructed code whose intent was to ensure that if a Python script invoked another Python script, the same interpreter would be used (#1828). This occurred because ondev, in the course of dropping Python2 support ondev(#2215), the script shebangs were changed to/usr/bin/python3rather than/usr/bin/env python3---note that this persists through #2741---but the code for explicit shebang parsing was not tested on such.I note that a reversion back to using
/usr/bin/envseems to be the most appropriate course of action from discussion with @daljit46. But this shebang parsing code is not just for MRtrix3 executables, it tries to choose the most suitable interpreter for any shebang.This change makes the relevant function more robust to both shebang styles, and enhances the version matching logic. If a script specifies a specific minor Python version (or indeed even patch version), the current interpreter will only be used if it is directly compatible with such; otherwise the shell will get to choose (including if the shell itself invokes
env).Tough to integrate the following as unit tests given that it depends on the contents of
PATH. So I'm just doing a dump here. For context, the system Python version is 3.10, but I'm explicitly running a 3.12.4 from outside ofPATH; this best shows the selection of when the current interpreter is run vs. other behaviour. Bold indicates cases where behaviour is different on Windows.Test behaviour on Windows.
In particular, the API supersedes
env, because this was requisite at some point in the past to allow Python scripts to execute other Python scripts on Windows. But this may have been an MSYS2 problem that has since been rectified.Discuss any contentious scenarios.
Possible that @daljit46 may disagree with some of these, preferring to allow the system to decide what to use in all scenarios. However the case that made me pursue this (Python API: Support Python 3.12 #2957) was specifically about my using a non-system version of Python yet having subsequent scripts then executing on the system Python. So my current intuition is to preserve this behaviour. The other one that might be contentious is what to do if just "
python" is specified as the destination interpreter. Here I'm invoking the current interpreter in that case.b'Not a shebang'b'\n#!/newline/before shebang'b'#!/usr/bin/env'b'#!/usr/bin/env python'b'#!/usr/bin/env python2'b'#!/usr/bin/env python3'b'#!/usr/bin/env python3.10'b'#!/usr/bin/env python3.10 extra'b'#!/usr/bin/env python3.11'b'#!/usr/bin/env python3.12'b'#!/usr/bin/env python3.12 extra'b'#!/usr/bin/env python3.10.1'b'#!/usr/bin/env python3.12.3'b'#!/usr/bin/env python3.12.4'b'#!/usr/bin/env python3.12.5'b'#!/usr/bin/env python3.x'b'#!/usr/bin/python'b'#!/usr/bin/python2'b'#!/usr/bin/python3'b'#!/usr/bin/python3.10'b'#!/usr/bin/python3.10 extra'b'#!/usr/bin/python3.11'b'#!/usr/bin/python3.12'b'#!/usr/bin/python3.12 extra'b'#!/usr/bin/python3.10.1'b'#!/usr/bin/python3.12.3'b'#!/usr/bin/python3.12.4'b'#!/usr/bin/blah'b'#!/usr/bin/foo bar'b'#!'b'#! /gap/in/shebang'