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

relenv breaks pip install #182

Closed
smarsching opened this issue May 17, 2024 · 5 comments · Fixed by #183
Closed

relenv breaks pip install #182

smarsching opened this issue May 17, 2024 · 5 comments · Fixed by #183
Assignees

Comments

@smarsching
Copy link

Recent versions of relenv (at least 0.15.0, 0.15.1, and 0.16.0) break pip install when used with the --target argument.

This causes salt-pip not to work correctly when trying to install some Python modules like mysqlclient (saltstack/salt#65980) or M2Crypto (saltstack/salt#66311).

I can definitely track this down to an issue in relenv because the problem can be resolved by commenting removing a single line in relenv’s runtime.py:

--- relenv/runtime.py.orig	2024-05-17 14:49:35.522975150 +0000
+++ relenv/runtime.py	2024-05-17 14:51:00.735990855 +0000
@@ -783,7 +783,6 @@
         Wrapper("pip._internal.operations.install.wheel", wrap_pip_install_wheel),
         Wrapper("pip._internal.operations.install.legacy", wrap_pip_install_legacy),
         Wrapper("pip._internal.operations.build.wheel", wrap_pip_build_wheel),
-        Wrapper("pip._internal.commands.install", wrap_cmd_install),
         Wrapper("pip._internal.locations", wrap_locations),
         Wrapper("pip._internal.cli.req_command", wrap_req_command),
         Wrapper("pip._internal.req.req_install", wrap_req_install),

As this might have undesired side effects (I do not understand the code well enough to be sure): I also tried more localized changes and came up with the following patch that resolves the issue:

--- relenv/runtime.py.orig	2024-05-17 14:49:35.522975150 +0000
+++ relenv/runtime.py	2024-05-17 14:53:50.545215677 +0000
@@ -641,19 +641,6 @@
 
     mod.InstallCommand.run = wrap(mod.InstallCommand.run)
 
-    def wrap(func):
-        @functools.wraps(func)
-        def wrapper(self, target_dir, target_temp_dir, upgrade):
-            from pip._internal.cli.status_codes import SUCCESS
-
-            return SUCCESS
-
-        return wrapper
-
-    if hasattr(mod.InstallCommand, "_handle_target_dir"):
-        mod.InstallCommand._handle_target_dir = wrap(
-            mod.InstallCommand._handle_target_dir
-        )
     return mod
 
 
@@ -670,8 +657,6 @@
         ):
             scheme = func(dist_name, user, home, root, isolated, prefix)
             if TARGET.TARGET:
-                scheme.platlib = TARGET.PATH
-                scheme.purelib = TARGET.PATH
                 scheme.data = TARGET.PATH
             return scheme
 
@@ -729,8 +714,6 @@
                 use_user_site=False,
                 pycompile=True,
             ):
-                if TARGET.TARGET:
-                    home = TARGET.PATH
                 return func(
                     self,
                     install_options,
@@ -756,8 +739,6 @@
                 use_user_site=False,
                 pycompile=True,
             ):
-                if TARGET.TARGET:
-                    home = TARGET.PATH
                 return func(
                     self,
                     global_options,

In order to resolve the error when trying to install the module, the change that avoids setting scheme.platlib and scheme.purelib is sufficient, but when only applying this change and not the other changes, the installed modules end up in /opt/saltstack/salt/extras-3.10/lib/python instead of /opt/saltstack/salt/extras-3.10.

When applying the change regarding scheme.purelib and home = TARGET.PATH, but not removing the wrapper for _handle_target_dir, there is no error but the module is not installed at all.

If I understand it correctly, home is usually set to a temporary directory and _handle_target_dir copies it from there to the actual target path. This is why not overriding home but replacing _handle_target_dir results in the module not being installed at all (it ends up in the temporary directory and is never moded from there).

@dwoz Unfortunately, I do not understand the intention behind this code, so I am not sure whether my fix is correct or might break something else. As it seems like you wrote most of this code, I hope that you might be able to explain what this code is supposed to do.

Maybe this code was targeted at an older version of pip, and due to changes in pip, it does not work correctly any longer (after all, the code messes with internals of pip). I tested my patch with Salt 3006.8, which bundles pip 23.3.2.

Some of the commits that added the code that is removed by my patch say something about fixing pip uninstall, but at fore the aforementioned version of pip, uninstall still works correctly after applying my patch.

@dwoz
Copy link
Contributor

dwoz commented Jun 6, 2024

@smarsching Can you confirm or deny that these packages will install successfully by adding the --no-build-isolation flag to you pip install command?

@dwoz
Copy link
Contributor

dwoz commented Jun 6, 2024

@smarsching Here is a tentative patch for the fix:

dwoz@def91f6

@smarsching
Copy link
Author

@Dowz I checked and adding the --no-build-isolation causes the installation process to succeed.

I also applied your patch to relenv 0.16.0, which is bundled with Salt 3006.8, and it does not fixe the issue for me:

root@489086526d19:~# salt-pip install --no-cache mysqlclient
Collecting mysqlclient
  Downloading mysqlclient-2.2.4.tar.gz (90 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 90.4/90.4 kB 37.2 MB/s eta 0:00:00
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  error: subprocess-exited-with-error
  
  × Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [14 lines of output]
      Error in sitecustomize; set PYTHONVERBOSE for traceback:
      AssertionError:
      Fatal Python error: init_import_site: Failed to import the site module
      Python runtime state: initialized
      Traceback (most recent call last):
        File "/opt/saltstack/salt/lib/python3.10/site.py", line 627, in <module>
          main()
        File "/opt/saltstack/salt/lib/python3.10/site.py", line 620, in main
          execsitecustomize()
        File "/opt/saltstack/salt/lib/python3.10/site-packages/relenv/runtime.py
", line 978, in wrapper
          import sitecustomize
        File "/tmp/pip-build-env-e8wklqti/site/sitecustomize.py", line 22, in <module>
          assert not path in sys.path
      AssertionError
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.

[notice] A new release of pip is available: 23.3.2 -> 24.0
[notice] To update, run: /opt/saltstack/salt/bin/python3.10 -m pip install --upgrade pip

I fixed the typo TARGET.ISNTALL => TARGET.INSTALL, but the install still failed. I also tried using all code from your branch instead of just applying the patch (to rule out that there is any interaction with other changes), but again the install failed.

@dwoz
Copy link
Contributor

dwoz commented Jun 8, 2024

@Dowz I checked and adding the --no-build-isolation causes the installation process to succeed.

I also applied your patch to relenv 0.16.0, which is bundled with Salt 3006.8, and it does not fixe the issue for me:

I fixed the typo TARGET.ISNTALL => TARGET.INSTALL, but the install still failed. I also tried using all code from your branch instead of just applying the patch (to rule out that there is any interaction with other changes), but again the install failed.

Okay, sorry to have you test a patch with typos. I've opened a PR with my changes and tests. Hopefully, these changes should work: #183

@smarsching
Copy link
Author

@dwoz I can confirm that #183 fixes the issue for me. Thank you for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants