[Fix] Package schema and request errors #658
Conversation
src/rez/utils/formatting.py
Outdated
| @@ -14,7 +14,7 @@ | |||
| import time | |||
|
|
|||
|
|
|||
| PACKAGE_NAME_REGSTR = "[a-zA-Z_0-9](\.?[a-zA-Z0-9_]+)*" | |||
| PACKAGE_NAME_REGSTR = "[a-zA-Z_0-9](\.?[a-zA-Z0-9_-]+)*" | |||
There was a problem hiding this comment.
Again I'm not quite clear on what's going on here.
The problem with this change is that in rez, - always separates the package name from its version. If a package name can contain hyphen, then which part is the name and which is the version becomes ambiguous.
There was a problem hiding this comment.
Hello Allan, yes this is true. This commit was from some time ago before I realized that rez uses dash to separate the package name from its version. After some tests it does not seem to be relevant to the SchemaError that occurs with some packages. It just happened to be a package with a dash in its name which is what made me think they are related.
I will revert this change and provide an example to the SchemaError.
Edit: Looking at the commit message for changing the REGEX I remembered that for whatever reason errors such as the one below used to occur
ERROR PackageRequestError: Not a valid package name: u'import-class'.
This is from formatting.py
is_valid = PACKAGE_NAME_REGEX.match(name)
if raise_error and not is_valid:
raise PackageRequestError("Not a valid package name: %r" % name)
return is_valid
Not sure what could have possibly changed that is not triggering the issue anymore but I guess adding the dash to the REGEX is not the way to fix it. I will keep my eyes open and if it occurs again I will make a new post.
rez-pip -i import-class
11:40:04 INFO Using pip-19.1.1 (/home/iakritas/packages/pip/19.1.1/package.py[0])
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Collecting import-class
Downloading https://files.pythonhosted.org/packages/14/b0/715a9ddb10f9da12abeebef28a3276ea29724e56ad8245e720ad99dca6d1/import_class-0.1.1-py2.py3-none-any.whl
Installing collected packages: import-class
Successfully installed import-class-0.1.1
11:40:06 DEBUG Removed 1.0 due to Classifier
11:40:06 DEBUG Found /tmp/pip-eH0KeY-rez/rez_staging/python/import_class-0.1.1.dist-info
1 packages were installed:
import_class-0.1.1: /home/iakritas/packages/import_class/0.1.1/package.py (platform-linux/arch-x86_64/os-Arch-rolling/python-2.7)
|
I'm a little lost on this one - I've gone over the conversation history on #628 but I can't see how these changes relate. Also might be good to re-merge with master now that #602 is merged, there are loads of diffs here that no longer apply and it's quite difficult to see which changes are specific to this PR. Thx! |
|
Re-merged as required! |
|
Worth looking into I think.
Related to this - if we are to obey pip package requirements to the letter,
that means taking markers into account (such as in this example). However,
that also implies that the rez package _must_ require/be varianted on at
least major.minor of the python version. It's possible to have a
requirement like "configparser>=3.5; python_version < '3'" in pip, because
the current python version is known ahead of time. Such requirement logic
isn't possible in rez however, because python is just another package. So
if a package has markers, the only way to satisfy the related requirements
is to have those requirements specified alongside the python version range
that requires them.
Here's where it gets tricky: what if you had a marker like so:
pathlib2; python_version=='3.4.1'
Now we're in trouble. The only way to express this requirement in rez would
be to have a variant on python-3.4.1, and that's getting a bit odd.
Furthermore, you might also have several markers, whose overlapped python
version requirements would cause even stranger varianting on python.
I'm not sure what the best solution is. It seems to me atm that the
considerable extra complexity needed to exactly define markered
requirements (ie, varianting the package on python in such a way that the
requirements are correct, but python version is never overspecified) is
more trouble than it's worth. If we just varianted on python-major.minor,
yes that would be overly specific for many cases, but it would also avoid
this problem.
…On Thu, Jul 11, 2019 at 1:20 PM Ira ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/rez/package_maker__.py
<#658 (comment)>:
> @@ -128,6 +128,10 @@ def get_package(self):
def _get_data(self):
data = self._data.copy()
+ if "requires" in data:
Looking at the importlib-metadata setup.cfg
<https://gitlab.com/python-devs/importlib_metadata/blob/master/setup.cfg>
file I can see the install requires being as follows:
install_requires =
zipp>=0.5
pathlib2; python_version=='3.4.*' or python_version < '3'
contextlib2; python_version < '3'
configparser>=3.5; python_version < '3'
Now looking at the requires above:
'requires': [u'zipp->=0.5', None, u'configparser->=3.5', None]
It seems to be missing two of its dependencies? pathlib2 and contextlib2
should have been included.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#658>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSTVKGYFAE2CA47VZB3P62RIFANCNFSM4H6XI7SA>
.
|
Fix the SchemaError "should be instance of" PackageRequest but is None. This is occuring when the requires list includes None and validation takes place.
Modify the allowed package name regex string to allow for packages that include a dash in their names which is quite common for Python packages. This eliminates issues like the one below: ERROR PackageRequestError: Not a valid package name: u'import-class'.
|
I'm pretty confident that #667 addresses this, so will close this PR when that is merged. |
|
Closing now, I'm certain that #667 fixes the issue, cheers |
Description
[Depends on PR #602 #654 #656]
This PR has been extracted from #628 (a lot of the relevant discussion still lives there)
Package version schema and request errors
Breakdown
Type of change
Test Configuration:
Todos