-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[master] Fix Python3.13 compatibility regarding urllib.parse module #66899
base: master
Are you sure you want to change the base?
Conversation
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.
Could you please write a test for this
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.
Needs tests written in pytest
The existing tests/unit/utils/test_url.py already covers this. And indeed, without my change, it fails already on Python 3.13rc2 (Fedora 41 beta) or Python 3.12.6 (Fedora 40). The change that broke did got backported to Python 3.12.6 (released about 2 weeks ago). I see you have Fedora 40 in CI already, but it seems to run use onedir (and Python 3.10.x inside) instead of Python from Fedora. Is there any CI job that tests on Python 3.12.x (or 3.13)? |
We currently only support up to Python 3.10. There are dependency issues we need to get figured out across all the Operating systems we support before we can officially support anything newer. |
In absence of newer Python to actually test the behavior there, do you want me to mimic its new behavior by patching BTW, does the CI report somewhere test coverage? From what I can see, coverage-related jobs got skipped... |
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.
how many "If/return"s you going to insert ?
note that changes in python code was introduced in minor release 3.12.6
try to make smart bulletproof improvements , for example
url = salt.utils.data.decode(urlunparse(("file", "", "/" + path, "", query, "")))
I think, just a unit test to make sure that if the URL starts with |
salt/utils/url.py
Outdated
|
||
query = f"saltenv={saltenv}" if saltenv else "" | ||
url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, ""))) | ||
return "salt://{}".format(url[len("file:///") :]) | ||
# urlunparse changed behavior in Python 3.13 | ||
if url.startswith("file:///"): | ||
return "salt://{}".format(url[len("file:///") :]) | ||
return "salt://{}".format(url[len("file:") :]) |
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.
This create function should not be doing path = path.replace("\\", /)
and probably not
if salt.utils.platform.is_windows():
path = salt.utils.path.sanitize_win_path(path)
Instead it should use urllib.request.pathname2url
or some other python builtin functionality.
Especially since the sanitize_win_path
function is clearly miswritten:
def sanitize_win_path(winpath):
"""
Remove illegal path characters for windows
"""
intab = "<>:|?*"
if isinstance(winpath, str):
winpath = winpath.translate({ord(c): "_" for c in intab})
elif isinstance(winpath, str):
Notice the duplicate isinstance(winpath, str)
in the elif that will never occur.
I would prefer instead of assuming that the string starts with file:
or file:///
we do a regex search replace like re.sub(r"^file:(?:///)?", "", url)
Also use f
string instead of format
.
return f'salt://{re.sub(r"^file:(?:///)?", "", url)}'
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.
return f'salt://{re.sub(r"^file:(?:/+)?", "", url)}'
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.
return f'salt://{re.sub(r"^file:(?:/+)?", "", url)}'
If you are going to do zero or more you don't +
and ?
nor parenthesis you just use *
return f'salt://{re.sub(r"^file:/*", "", url)}'
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.
and if you want to do 0 to 3 /
then it would be
return f'salt://{re.sub(r"^file:/{0,3}", "", url)}'
or
return f'salt://{re.sub(r"^file:/{,3}", "", url)}'
but I think the explicit 0
is more readable.
Most tests in tests/unit/utils/test_url.py do that already, for example |
salt/utils/url.py
Outdated
@@ -47,7 +47,7 @@ def create(path, saltenv=None): | |||
|
|||
query = f"saltenv={saltenv}" if saltenv else "" | |||
url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, ""))) | |||
return "salt://{}".format(url[len("file:///") :]) | |||
return f'salt://{re.sub(r"file:/*", "", url)}' |
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 regex needs a begin anchor ^
so it doesn't replace file:/
somewhere in the middle of the url: f'salt://{re.sub(r"^file:/*", "", url)}'
Python 3.13 fixed handling relative paths in urllib.parse module. Specifically, relative file URL is now constructed as file:path instead of converting it to absolute file:///path. This breaks salt.utils.url.create which expects file:/// specifically. The mismatch results in for example changing salt://top.sls into salt://.sls and thus not finding the top file. Fix this by handling both prefixes. Relevant python change: python/cpython#85110 Fixes: saltstack#66898
What does this PR do?
Python 3.13 fixed handling relative paths in
urllib.parse
module. Specifically, relative file URL is now constructed asfile:path
instead of converting it to absolutefile:///path
. This breakssalt.utils.url.create
which expectsfile:///
specifically. The mismatch results in for example changingsalt://top.sls
intosalt://.sls
and thus not finding the top file.Fix this by handling both prefixes.
Relevant python change: python/cpython#85110
What issues does this PR fix or reference?
Fixes: #66898
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes
Please review Salt's Contributing Guide for best practices, including the
PR Guidelines.
See GitHub's page on GPG signing for more information about signing commits with GPG.