Skip to content
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

Rez pip_install_remaps need to escape sep #1409

Open
hughetop opened this issue Nov 17, 2022 · 7 comments
Open

Rez pip_install_remaps need to escape sep #1409

hughetop opened this issue Nov 17, 2022 · 7 comments
Labels
bug rez-pip ingesting py pkgs into rez (pip, wheels, etc)

Comments

@hughetop
Copy link

The pip_install_remaps feature works great for things on linux, but I'm running into an issue on windows that seems to be due to the backslash directory separator. When you rez-pip install networkx, it comes with a bunch of docs and examples. I've remapped them to copy to a "docs" directory next to the "python" directory. However, this doesn't work on windows due to os.sep being confused for a matching group in regex.

Environment

  • OS windows 10
  • Rez version 2.111.3
  • Rez python version 3.9

To Reproduce

  1. Make sure the rezconfig.py file has a pip_install_remaps with:
{
    "record_path": r"{pardir}{sep}{pardir}{sep}share{sep}doc{sep}networkx-([\w\.]+){sep}(.*)",
    "pip_install": r"share{sep}doc{sep}networkx-\1{sep}\2",
    "rez_install": r"doc{sep}\2",
},
  1. rez-pip -i networkx

Expected behavior
networkx should install and the docs should be remapped

Actual behavior
An error occurs during the install process.

Traceback (most recent call last):
  File "C:\Program Files\Python39\lib\sre_parse.py", line 1051, in parse_template
    this = chr(ESCAPES[this][1])
KeyError: '\\d'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Program Files\Python39\lib\runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Program Files\Python39\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "C:\rez\Scripts\rez\rez-pip.exe\__main__.py", line 7, in <module>
  File "c:\rez\lib\site-packages\rez\cli\_entry_points.py", line 183, in run_rez_pip
    return run("pip")
  File "c:\rez\lib\site-packages\rez\cli\_main.py", line 191, in run
    returncode = run_cmd()
  File "c:\rez\lib\site-packages\rez\cli\_main.py", line 183, in run_cmd
    return func(opts, opts.parser, extra_arg_groups)
  File "c:\rez\lib\site-packages\rez\cli\pip.py", line 67, in command
    pip_install_package(
  File "c:\rez\lib\site-packages\rez\pip.py", line 366, in pip_install_package
    src_dst_lut = _get_distribution_files_mapping(distribution, targetpath)
  File "c:\rez\lib\site-packages\rez\pip.py", line 565, in _get_distribution_files_mapping
    rel_src, rel_dest = get_mapping(rel_src_orig)
  File "c:\rez\lib\site-packages\rez\pip.py", line 515, in get_mapping
    pip_subpath = re.sub(path, remap['pip_install'], rel_src)
  File "C:\Program Files\Python39\lib\re.py", line 210, in sub
    return _compile(pattern, flags).sub(repl, string, count)
  File "C:\Program Files\Python39\lib\re.py", line 327, in _subx
    template = _compile_repl(template, pattern)
  File "C:\Program Files\Python39\lib\re.py", line 318, in _compile_repl
    return sre_parse.parse_template(repl, pattern)
  File "C:\Program Files\Python39\lib\sre_parse.py", line 1054, in parse_template
    raise s.error('bad escape %s' % this, len(this))
re.error: bad escape \d at position 5

In rez/pip.py above line 515, I added extra logging to see what path, remap['pip_install'], and rel_src are:

17:20:33 DEBUG    path: \.\.\\\.\.\\share\\doc\\networkx-([\w\.]+)\\(.*)
17:20:33 DEBUG    pip_install: share\doc\networkx-\1\\2
17:20:33 DEBUG    rel_src: ..\..\share\doc\networkx-2.8.8\LICENSE.txt

Notice on the 2nd line, the backslashes are not escaped, so regex thinks that \d is a matching group.

As a test, I opened up rez/config.py and changed line 143 to:

key: expression.format(**(self.RE_TOKENS))

That way the pip_install and rez_install values in the pip_install_remaps have their os.sep escaped and the remap works.

@hughetop hughetop added the bug label Nov 17, 2022
@JeanChristopheMorinPerso JeanChristopheMorinPerso added the rez-pip ingesting py pkgs into rez (pip, wheels, etc) label Nov 17, 2022
@instinct-vfx
Copy link
Contributor

Thanks for the report. As a side note: I am not a fan of how pip_install_remap works now as it forces us to add package specific (or better: files in packages specific) configuration to global config. It is also quite a lot of hassle everytime we encounter a package that requires it. I would rather have sensible defaults for files with unknown locations, output a warning and make pip_install_remap optional for those shops (or packages) that require it.

@hughetop
Copy link
Author

Sorry if that seemed like rambling, I was trying to type it all up before the end of the work day.

I agree, it's a bit confusing and (in this case) specific to one problematic package. In this case, it's docs examples, which is great, but unnecessary. Unfortunately there was no way to disable or skip that step, so I had to use the remap feature. Again, it worked fine on linux, just not windows because of the backslashes.

Instead of adding package-specific logic to global configs, I think the option to ignore those errors would suffice for most things. Aside from the built-in bin relocation, most of the troublesome packages include docs that aren't 100% necessary. So being able to tell rez to skip that part would be fine for me. Of course this would need to be addressed per package, since the user will need to determine if the extra files are needed or not.

@instinct-vfx
Copy link
Contributor

Ah no problem. I just wanted to double down and state that i am generally not a fan :)

@JeanChristopheMorinPerso
Copy link
Member

This ticket was mentioned in the TSC meeting and I also expressed that pip_install_remap is currently problematic and would need to be revisited.

Also, as a side note, I think that generally speaking, packages that install stuff outside the site-packages directory (excluding the entry points) are kind of badly packaged.

@hughetop
Copy link
Author

Also, as a side note, I think that generally speaking, packages that install stuff outside the site-packages directory (excluding the entry points) are kind of badly packaged.

I agree 100%.

@brycegbrazen
Copy link
Contributor

+1 as something I'm still coming across. On Windows Rez breaks with issues with escaped characters.

Also, +1 to the concept of seeing what exactly is causing the remapping to be necessary, and for an option to skip it if it's not important.

@JeanChristopheMorinPerso
Copy link
Member

This will be gone in the new rez-pip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug rez-pip ingesting py pkgs into rez (pip, wheels, etc)
Projects
None yet
Development

No branches or pull requests

4 participants