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

resolve loader_path #100

Closed
wants to merge 11 commits into from
Closed

resolve loader_path #100

wants to merge 11 commits into from

Conversation

isuruf
Copy link
Collaborator

@isuruf isuruf commented Feb 28, 2021

No description provided.

@matthew-brett
Copy link
Owner

How did @loader_path get into the Numpy wheels?

We need some tests...

Does the Numpy wheel work once this patch is applied?

@isuruf
Copy link
Collaborator Author

isuruf commented Feb 28, 2021

How did @loader_path get into the Numpy wheels?

It didn't get into the numpy wheels. It's in libgfortran.dylib from the gfortran-darwin-arm64 toolchain.

Does the Numpy wheel work once this patch is applied?

It should. I haven't checked it.

We need some tests...

I've no idea how to write a test. Any suggestions?

@matthew-brett
Copy link
Owner

matthew-brett commented Mar 3, 2021

Aha - I don't we had previously seen @loader_path inside an rpath.

For the tests - I'm afraid it's a little bit fiddly. But you could do a first pass by extending the tests in test_add_rpath from delocate/tests/test_install_names.py...

@matthew-brett
Copy link
Owner

Nice - thanks. Just to do the final sign-off - would you mind checking if the Numpy wheels work with this fix?

@isuruf
Copy link
Collaborator Author

isuruf commented Mar 3, 2021

It seems like we are running this function after copying libgfortran.dylib to the wheel and therefore resolving @loader_path doesn't give the intended path.

Copy link
Owner

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass thoughts. I need to reflect a bit on the new argument.

delocate/libsana.py Outdated Show resolved Hide resolved
@@ -141,6 +141,11 @@ def test_add_rpath():
assert_equal(get_rpaths(libfoo), ('/a/path',))
add_rpath(libfoo, '/another/path')
assert_equal(get_rpaths(libfoo), ('/a/path', '/another/path'))
add_rpath(libfoo, '@loader_path')
assert_equal(get_rpaths(libfoo), ('/a/path', '/another/path', tmpdir))
add_rpath(libfoo, '@loader_path/lib')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this now testing the original path manipulation?

@@ -56,7 +58,16 @@ def tree_libs(start_path, filt_func=None):
depending_libpath = realpath(pjoin(dirpath, base))
if filt_func is not None and not filt_func(depending_libpath):
continue
rpaths = get_rpaths(depending_libpath)

depending_libpath_orig = depending_libpath
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments would help here.

delocate/delocating.py Outdated Show resolved Hide resolved
@isuruf
Copy link
Collaborator Author

isuruf commented Apr 27, 2021

Ping on this

delocate/libsana.py Outdated Show resolved Hide resolved
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@@ -180,6 +180,9 @@ def _cmd_out_err(cmd):
return out.split('\n')


_LINE0_RE = re.compile(r"^(?: \(architecture .*\))?:(?P<further_report>.*)")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind splitting these changes off into a separate PR, after #94 is merged? Maybe add a comment above this line explaining the "architecture" string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll send a PR after #94 is merged.

@matthew-brett
Copy link
Owner

Can we close this one now #94 is merged and you've factored out #115 ?

@isuruf
Copy link
Collaborator Author

isuruf commented Jul 16, 2021

Yes

@isuruf isuruf closed this Jul 16, 2021
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 this pull request may close these issues.

None yet

3 participants