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

Scripts are not correctly repaired #340

Closed
FFY00 opened this issue Sep 29, 2021 · 13 comments · Fixed by #443
Closed

Scripts are not correctly repaired #340

FFY00 opened this issue Sep 29, 2021 · 13 comments · Fixed by #443

Comments

@FFY00
Copy link
Member

FFY00 commented Sep 29, 2021

Binaries that go to the .data/scripts folder are not fixed correctly, the rpath value is wrong. There's no way to know the correct rpath value if we keep the executable in the scripts folder, as that depends on the Python installation.

The only solution I see is to move the scripts to platlib, where we know its relative path to the .libs folder, and place a wrapper in its place that calls os.execv on the actual binary.

Does anyone have other ideas?

See mesonbuild/meson-python#3 (comment)

@FFY00 FFY00 changed the title Scripts are not fixed Scripts are not correctly repaired Sep 29, 2021
@mayeut
Copy link
Member

mayeut commented Oct 3, 2021

I made a similar observation in #257 (comment).

The only solution I see is to move the scripts to platlib, where we know its relative path to the .libs folder, and place a wrapper in its place that calls os.execv on the actual binary.

This seems like the most sensible thing to do. If there are only scripts, we might be able to graft libraries to go in the .data/scripts (if that's allowed, remains to be checked) rather than putting them in platlib/root.

@FFY00
Copy link
Member Author

FFY00 commented Oct 3, 2021

That would work but it's an ugly solution IMO 😅. I do not really see a reason to not relocate the scripts to platlib in that situation too, perhaps I am missing something.

@mayeut
Copy link
Member

mayeut commented Oct 3, 2021

I do not really see a reason to not relocate the scripts to platlib in that situation too, perhaps I am missing something.

If I understand correctly:
If we find ELF files in scripts, move them to platlib & re-write scripts ?

@FFY00
Copy link
Member Author

FFY00 commented Oct 3, 2021

That'd be my proposal, yes.

@mayeut
Copy link
Member

mayeut commented Dec 18, 2021

we're between bug & enhancement. I'd say it was a limitation that scripts could not be repaired & marked as enhancements.

@FFY00
Copy link
Member Author

FFY00 commented Dec 18, 2021

But auditwheel does try to do something currently.

@mayeut mayeut added bug and removed enhancement labels Dec 19, 2021
@mayeut
Copy link
Member

mayeut commented Dec 19, 2021

But auditwheel does try to do something currently.

you're right, relabeling. All ELF files are treated the same which might prevent a work around.

@chriskuehl
Copy link
Contributor

Would there still be interest in fixing this if I could make a PR?

I've run into this recently when converting some wheels and have a (draft, quite rough) patch based on the ideas suggested in this issue. It works by making a new ${package}.scripts directory in the wheel (similar to the ${package}.libs directory) and moving the ELF binary in there, then putting a wrapper script in.data/scripts which execs it:

diff --git a/src/auditwheel/repair.py b/src/auditwheel/repair.py
index 23f172e..d7d3b2c 100644
--- a/src/auditwheel/repair.py
+++ b/src/auditwheel/repair.py
@@ -32,6 +32,34 @@ WHEEL_INFO_RE = re.compile(
 ).match
 
 
+def _path_is_script(path: str) -> bool:
+    # Looks something like "uWSGI-2.0.21.data/scripts/uwsgi"
+    components = path.split("/")
+    return (
+        len(components) == 3 and
+        components[0].endswith(".data") and
+        components[1] == "scripts"
+    )
+
+
+SCRIPT_WRAPPER = """\
+#!python
+import os
+import sys
+import sysconfig
+
+
+BINARY_PATH = {BINARY_PATH!r}
+
+
+if __name__ == "__main__":
+    os.execv(
+        os.path.join(sysconfig.get_path("platlib"), BINARY_PATH),
+        sys.argv,
+    )
+"""
+
+
 def repair_wheel(
     wheel_path: str,
     abis: list[str],
@@ -92,6 +120,39 @@ def repair_wheel(
                 patcher.replace_needed(fn, *replacements)
 
             if len(ext_libs) > 0:
+                if _path_is_script(fn):
+                    scripts_dir = match.group("name") + ".scripts"
+                    if not exists(scripts_dir):
+                        os.mkdir(scripts_dir)
+
+                    orig_path = os.path.join(ctx.name, fn)
+                    new_path = os.path.join(scripts_dir, os.path.basename(fn))
+                    os.rename(orig_path, new_path)
+                    fn = os.path.relpath(new_path, ctx.name)
+
+                    with open(orig_path, "w") as f:
+                        f.write(SCRIPT_WRAPPER.format(BINARY_PATH=new_path))
+                    os.chmod(orig_path, os.stat(orig_path).st_mode | stat.S_IXUSR)
+
                 new_rpath = os.path.relpath(dest_dir, os.path.dirname(fn))
                 new_rpath = os.path.join("$ORIGIN", new_rpath)
                 append_rpath_within_wheel(fn, new_rpath, ctx.name, patcher)

I've tested this with a few packages (e.g. uwsgi) and it seems to work well, but would appreciate any thoughts as I might be missing something.

If you think this kind of approach is workable, I can clean up this patch and turn it into a proper PR?

@auvipy
Copy link
Contributor

auvipy commented Aug 27, 2023

You can come with a draft PR!

@robtaylor
Copy link

Happy to help out on this, definitely a need i have with my current project

@chriskuehl
Copy link
Contributor

I created a draft PR, feedback would be welcome at #443

@henryiii
Copy link

henryiii commented Jun 22, 2024

This is breaking user installs. If it's installed to --user, then it's getting the path to the system platlib which is incorrect. aestream/aestream#112

Can't users just do this themselves and provide a console entrypoint? Isn't this a problem on macOS and Windows too? And writing a wrapper can cause issues with file descriptors and adds the Python start up time to the binary, which it seems is better handled by the user manually using a console entry point instead of being rewritten without user control?

If this is only rewriting ones with links, then I guess it's never breaking something that would work, so I think that is not as bad, but it should get the correct platlib.

@FFY00
Copy link
Member Author

FFY00 commented Oct 23, 2024

There's no way of identifying the correct platlib when we are generating/fixing a wheel, only at runtime, as on posix_user both platlibs are valid.

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

Successfully merging a pull request may close this issue.

6 participants