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

MAINT: skip setting rpath if it's not needed #139

Merged
merged 5 commits into from
Sep 28, 2022
Merged

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Sep 13, 2022

I also fixed ELF.rpath, which is needed for the tests, and added a test for setting the rpath in the first place in this PR.

Fixes #125

@FFY00 FFY00 requested a review from rgommers September 13, 2022 22:31
@FFY00 FFY00 force-pushed the avoid-uneeded-rpath branch from 4285ccb to 049b439 Compare September 13, 2022 23:36
Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @FFY00. There are some issues shown by the CI results. The code change looks like an improvement, and will solve the most important part of gh-125. It looks like it won't yet solve the unnecessary dependency issue. That can be left for later if you prefer.

tests/test_wheel.py Show resolved Hide resolved
mesonpy/_elf.py Show resolved Hide resolved
@rgommers rgommers changed the title ENH: skip setting rpath if uneeded MAINT: skip setting rpath if it's not needed Sep 27, 2022
@rgommers rgommers added the maintenance Regular code improvements that are not new features nor end-user-visible bugs label Sep 27, 2022
@rgommers
Copy link
Contributor

Ugh, this still depends on other fixes:

patchelf: getting info about '/tmp/pytest-of-runner/pytest-0/test_unneeded_r"
 b"path0/plat.cpython-39-x86_64-linux-gnu.so': No such file or directory

That's because the test package it's built on is still xfailed:

@pytest.mark.xfail(reason='Meson bug')
def test_purelib_and_platlib(wheel_purelib_and_platlib):
    artifact = wheel.wheelfile.WheelFile(wheel_purelib_and_platlib)

    expecting = {
        f'plat{EXT_SUFFIX}',

This passes locally with Meson master. After the other open PR for tags is merged, it should pass with released Meson as well.

@rgommers rgommers added this to the v0.9.0 milestone Sep 27, 2022
@FFY00 FFY00 force-pushed the avoid-uneeded-rpath branch from e30111c to 914c1f4 Compare September 28, 2022 15:34
@FFY00
Copy link
Member Author

FFY00 commented Sep 28, 2022

As per https://github.com/FFY00/meson-python/pull/143#issuecomment-1261084168, I rebased this PR on main to get rid of the non-linear commit history as I'd prefer the PR to be rebase-merged.

@FFY00 FFY00 force-pushed the avoid-uneeded-rpath branch from 914c1f4 to 167d25e Compare September 28, 2022 15:41
@rgommers
Copy link
Contributor

You just lost the commits that made this PR pass CI. I'll re-add them.

@FFY00 FFY00 force-pushed the avoid-uneeded-rpath branch from 167d25e to adda659 Compare September 28, 2022 15:55
@FFY00
Copy link
Member Author

FFY00 commented Sep 28, 2022

Yeah, sorry, they got lost in the rebase. I have just added the fixes directly to the affected commits.

@FFY00
Copy link
Member Author

FFY00 commented Sep 28, 2022

Sorry 😅

@FFY00 FFY00 force-pushed the avoid-uneeded-rpath branch from adda659 to c342874 Compare September 28, 2022 16:05
@rgommers
Copy link
Contributor

I think that's it, but hard to be sure since I had to manually re-apply fixes. Let's first ensure CI is green, and then if desired at the end rewrite history to the desired commit granularity.

FFY00 and others added 4 commits September 28, 2022 17:49
Signed-off-by: Filipe Laíns <[email protected]>
Signed-off-by: Filipe Laíns <[email protected]>
This allows figuring out what went wrong, in case there is an
issue.

Signed-off-by: Filipe Laíns <[email protected]>
@FFY00
Copy link
Member Author

FFY00 commented Sep 28, 2022

I have squashed the fix with the affected commit, the tests seem to be passing, are you okay with me pushing?

@rgommers
Copy link
Contributor

Sure, go ahead. CI is green (codecov isn't relevant, and Sage isn't currently testing the right thing). So rewrite history as you wish and then merge it I'd say.

@FFY00 FFY00 force-pushed the avoid-uneeded-rpath branch from 101991a to b7d251a Compare September 28, 2022 17:05
@FFY00 FFY00 merged commit 61fa334 into main Sep 28, 2022
@FFY00 FFY00 deleted the avoid-uneeded-rpath branch September 28, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Regular code improvements that are not new features nor end-user-visible bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only insert RUNPATH entries if that is actually needed
2 participants