Refine rpath fixup to inspect already existing rpaths #36056

Neumann-A · 2024-01-05T21:21:50Z

Reason:

-- Adjusting original rpath of: '/mnt/e/all/vcpkg/installed/x64-linux-release/lib/pkgconfig/../../lib/intel64/'
-- Fixed rpath in: '/mnt/e/all/vcpkg/packages/numpy_x64-linux-release/lib/python3.11/site-packages/numpy/linalg/lapack_lite.cpython-311-x86_64-linux-gnu.so' ('$ORIGIN:$ORIGIN/../../../../:$ORIGIN/../../../../intel64/')

needed $ORIGIN/../../../../intel64/ in there which came from a pc file via -Wl,-rpath='${libdir}'

Cheney-W · 2024-01-08T09:23:11Z

/azp run

azure-pipelines · 2024-01-08T09:23:21Z

Azure Pipelines successfully started running 1 pipeline(s).

scripts/cmake/z_vcpkg_fixup_rpath.cmake

Neumann-A · 2024-01-14T19:14:58Z

@dg0yt Any idea why vcpkg-find-acquire-program is failing or why it does a double extraction (at least that is what I assume)? Is the search path wrong? Should the test port search for everything twice?

dg0yt · 2024-01-14T19:32:38Z

I guess the problem is related to the fact that vfap is unable to parse the version from the program's output, and that's why it even doesn't accept its own previous download.
Cf. #36046.

Cheney-W · 2024-02-02T09:06:43Z

/azp run

azure-pipelines · 2024-02-02T09:06:52Z

Azure Pipelines successfully started running 1 pipeline(s).

Neumann-A · 2024-02-04T19:42:44Z

I guess the problem is related to the fact that vfap is unable to parse the version from the program's output, and that's why it even doesn't accept its own previous download.
Cf. #36046.

Nope was a problem with the test. It simply set the variable to empty resulting in unexpected behavior

triplets/x64-linux.cmake

BillyONeal

LGTM with string manipulation test(s)

scripts/cmake/z_vcpkg_fixup_rpath.cmake

Osyotr

Can we have x64-linux-dynamic CI run with these changes?

Osyotr · 2024-02-11T19:55:07Z

scripts/ports.cmake

@@ -169,8 +171,8 @@ if(CMD STREQUAL "BUILD")

 include("${CURRENT_PORT_DIR}/portfile.cmake")
 if(DEFINED PORT)
- if(VCPKG_FIXUP_ELF_RPATH)
- include("${SCRIPTS}/cmake/z_vcpkg_fixup_rpath.cmake")
+ if(VCPKG_FIXUP_ELF_RPATH OR VCPKG_TARGET_IS_LINUX)


Suggested change

if(VCPKG_FIXUP_ELF_RPATH OR VCPKG_TARGET_IS_LINUX)

if(VCPKG_FIXUP_ELF_RPATH)

or

Suggested change

if(VCPKG_FIXUP_ELF_RPATH OR VCPKG_TARGET_IS_LINUX)

if(VCPKG_FIXUP_ELF_RPATH AND VCPKG_TARGET_IS_LINUX)

Otherwise I don't see how VCPKG_FIXUP_ELF_RPATH makes sense anymore.

#36056 (comment)

I think I was assuming it would be fixed to not apply to static triplets in my comment.

scripts/cmake/z_vcpkg_fixup_rpath.cmake

triplets/x64-linux.cmake

Osyotr · 2024-02-11T20:11:22Z

scripts/cmake/z_vcpkg_fixup_rpath.cmake

 execute_process(
- COMMAND "${PATCHELF}" --set-rpath "${rpath}" "${elf_file}"
+ COMMAND "${PATCHELF}" --set-rpath "${new_rpath}" "${elf_file}"
 OUTPUT_QUIET
 ERROR_VARIABLE set_rpath_error


Suggested change

ERROR_VARIABLE set_rpath_error

Or keep the error check in case patchelf for some reason couldn't fix our binary for some reason.

Co-authored-by: Osyotr <Osyotr@users.noreply.github.com>

Cheney-W · 2024-02-18T07:03:25Z

/azp run

azure-pipelines · 2024-02-18T07:03:34Z

Azure Pipelines successfully started running 1 pipeline(s).

Neumann-A · 2024-02-21T14:30:15Z

Should I retrigger CI or should i wait for further review?

Cheney-W · 2024-02-22T07:40:16Z

/azp run

azure-pipelines · 2024-02-22T07:40:25Z

Azure Pipelines successfully started running 1 pipeline(s).

…e actually need it. In microsoft#36056 , things were changed to unconditionally patchelf when targeting Linux. Unfortunately that broke --only-downloads a lot of the time. This fixes that.

* print compiler path * update messages.json * fix empty message * fix format * add tests and fix * update vcpkg to latest commit * fix * fix * Fix e2e test on Windows. * Change scripts SHA to not include microsoft/vcpkg#36056 --------- Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>

@Osyotr

…ATH (#37129) ... and don't fetch patchelf unless we actually need it. For example cmake helper ports have no need to fetch patchelf. In #36056 , things were changed to unconditionally patchelf when targeting Linux. Unfortunately that broke --only-downloads a lot of the time. This fixes that. Also contains @Osyotr 's fixes from #37190 to coalesce world rebuilds a bit, so repeating comments from there: Fixes #37183 Towards #25668

dg0yt · 2024-04-05T16:40:53Z

scripts/cmake/z_vcpkg_fixup_rpath.cmake

+ # lib relative corrections
+ list(TRANSFORM rpath_norm REPLACE "${current_prefix}/lib/?" "\$ORIGIN/${relative_to_lib}/")
+ list(TRANSFORM rpath_norm REPLACE "${current_installed_prefix}/lib/?" "\$ORIGIN/${relative_to_lib}/")
+ list(TRANSFORM rpath_norm REPLACE "${current_prefix}/lib/?" "\$ORIGIN/${relative_to_lib}/")
+ list(TRANSFORM rpath_norm REPLACE "${current_installed_prefix}/lib/?" "\$ORIGIN/${relative_to_lib}/")
+ # prefix relativ
+ list(TRANSFORM rpath_norm REPLACE "${current_prefix}" "\$ORIGIN/${relative_to_prefix}/")
+ list(TRANSFORM rpath_norm REPLACE "${current_installed_prefix}" "\$ORIGIN/${relative_to_prefix}/")
+ list(TRANSFORM rpath_norm REPLACE "${current_prefix}" "\$ORIGIN/${relative_to_prefix}/")
+ list(TRANSFORM rpath_norm REPLACE "${current_installed_prefix}" "\$ORIGIN/${relative_to_prefix}/")


@Neumann-A I may be overlooking the obvious: Is there a reason why each line occurs twice? AFAICS they cannot match twice. Every match inserts $ORIGIN, and then it cannot have a second insertion.

Neumann-A added 3 commits January 5, 2024 21:59

Refine z_vcpkg_fixup_rpath to take existing rpath settings into account

921afab

Force rpath fixup in normal linux triplets

ccac791

need to also replace lib/

f864724

Neumann-A mentioned this pull request Jan 6, 2024

Always setup rpath #27426

Closed

Merge remote-tracking branch 'upstream/master' into refine_rpath_fixup

df653e0

JonLiu1993 assigned Cheney-W Jan 8, 2024

Cheney-W added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jan 8, 2024

Osyotr reviewed Jan 8, 2024

View reviewed changes

scripts/cmake/z_vcpkg_fixup_rpath.cmake Outdated Show resolved Hide resolved

use patchelf to read rpath

9095755

Neumann-A mentioned this pull request Jan 13, 2024

Using / collaborating on python ports Neumann-A/my-vcpkg-ports#2

Closed

Neumann-A added 2 commits January 14, 2024 23:16

replace trailing \n and /

ce7ff23

Merge branch 'microsoft:master' into refine_rpath_fixup

ca4fa57

fix testcase modifying the variables.

2b2cb39

Cheney-W previously approved these changes Feb 8, 2024

View reviewed changes

Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Feb 8, 2024

data-queue previously approved these changes Feb 10, 2024

View reviewed changes

BillyONeal reviewed Feb 10, 2024

View reviewed changes

triplets/x64-linux.cmake Outdated Show resolved Hide resolved

BillyONeal reviewed Feb 10, 2024

View reviewed changes

scripts/cmake/z_vcpkg_fixup_rpath.cmake Outdated Show resolved Hide resolved

scripts/cmake/z_vcpkg_fixup_rpath.cmake Outdated Show resolved Hide resolved

Neumann-A added 3 commits February 10, 2024 11:57

apply suggestion from CR

52b16cd

add test port

c4c7179

revert linux triplet

2fbead5

Neumann-A dismissed stale reviews from data-queue and Cheney-W via 2fbead5 February 10, 2024 10:58

Neumann-A added 2 commits February 10, 2024 12:02

revert community triplet

9e2360d

add another line

0d84b70

Osyotr reviewed Feb 11, 2024

View reviewed changes

remove ws

bf77727

Co-authored-by: Osyotr <Osyotr@users.noreply.github.com>

Cheney-W removed the info:reviewed Pull Request changes follow basic guidelines label Feb 18, 2024

Cheney-W approved these changes Feb 27, 2024

View reviewed changes

Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Feb 27, 2024

vicroms merged commit 2ac6ba1 into microsoft:master Feb 29, 2024
16 checks passed

Neumann-A deleted the refine_rpath_fixup branch February 29, 2024 06:56

BillyONeal mentioned this pull request Mar 4, 2024

Allow uname calls in download mode + Fix handling of files without RPATH #37129

Merged

BillyONeal added a commit to data-queue/vcpkg-tool that referenced this pull request Mar 4, 2024

Change scripts SHA to not include microsoft/vcpkg#36056

b1248c5

Osyotr mentioned this pull request Mar 6, 2024

[invalid] Test x64-linux-dynamic #37190

Closed

dg0yt mentioned this pull request Apr 5, 2024

[python3] Build error on x64-linux #37984

Closed

dg0yt reviewed Apr 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine rpath fixup to inspect already existing rpaths #36056

Refine rpath fixup to inspect already existing rpaths #36056

Neumann-A commented Jan 5, 2024

Cheney-W commented Jan 8, 2024

azure-pipelines bot commented Jan 8, 2024

Neumann-A commented Jan 14, 2024

dg0yt commented Jan 14, 2024

Cheney-W commented Feb 2, 2024

azure-pipelines bot commented Feb 2, 2024

Neumann-A commented Feb 4, 2024

BillyONeal left a comment

Osyotr left a comment

Osyotr Feb 11, 2024

Neumann-A Feb 12, 2024

BillyONeal Mar 4, 2024

Osyotr Feb 11, 2024

Cheney-W commented Feb 18, 2024

azure-pipelines bot commented Feb 18, 2024

Neumann-A commented Feb 21, 2024

Cheney-W commented Feb 22, 2024

azure-pipelines bot commented Feb 22, 2024

dg0yt Apr 5, 2024

	if(VCPKG_FIXUP_ELF_RPATH OR VCPKG_TARGET_IS_LINUX)
	if(VCPKG_FIXUP_ELF_RPATH)

	if(VCPKG_FIXUP_ELF_RPATH OR VCPKG_TARGET_IS_LINUX)
	if(VCPKG_FIXUP_ELF_RPATH AND VCPKG_TARGET_IS_LINUX)

Refine rpath fixup to inspect already existing rpaths #36056

Refine rpath fixup to inspect already existing rpaths #36056

Conversation

Neumann-A commented Jan 5, 2024

Cheney-W commented Jan 8, 2024

azure-pipelines bot commented Jan 8, 2024

Neumann-A commented Jan 14, 2024

dg0yt commented Jan 14, 2024

Cheney-W commented Feb 2, 2024

azure-pipelines bot commented Feb 2, 2024

Neumann-A commented Feb 4, 2024

BillyONeal left a comment

Choose a reason for hiding this comment

Osyotr left a comment

Choose a reason for hiding this comment

Osyotr Feb 11, 2024

Choose a reason for hiding this comment

Neumann-A Feb 12, 2024

Choose a reason for hiding this comment

BillyONeal Mar 4, 2024

Choose a reason for hiding this comment

Osyotr Feb 11, 2024

Choose a reason for hiding this comment

Cheney-W commented Feb 18, 2024

azure-pipelines bot commented Feb 18, 2024

Neumann-A commented Feb 21, 2024

Cheney-W commented Feb 22, 2024

azure-pipelines bot commented Feb 22, 2024

dg0yt Apr 5, 2024

Choose a reason for hiding this comment