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

[vcpkg] Fixup rpath after building dynamic libraries on linux #23035

Merged
merged 3 commits into from
Jul 18, 2022

Conversation

Osyotr
Copy link
Contributor

@Osyotr Osyotr commented Feb 10, 2022

This is required to make packages built with (currently nonexisting) x64-linux-dynamic triplet relocatable.
After building a package, we patch binaries so that they have correct rpath values, relative to lib/ (or debug/lib/).
Partially addresses my comment #19940 (comment).

github-actions[bot]
github-actions bot previously approved these changes Feb 10, 2022
@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Feb 11, 2022
@JackBoosY
Copy link
Contributor

We are still discussing about rpath in #19127.

@Osyotr
Copy link
Contributor Author

Osyotr commented Feb 11, 2022

@JackBoosY six months without replies is not really a discussion... Maybe this implementation will reignite it.
PS: is there a way to disable automatic checks on commit? They are useless for this PR until x64-linux-dynamic is added.

@JackBoosY
Copy link
Contributor

Fine, I canceled all tests.

@dg0yt
Copy link
Contributor

dg0yt commented Feb 11, 2022

Fine, I canceled all tests.

Why? It may still break CI.

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Osyotr

This comment was marked as resolved.

@JackBoosY
Copy link
Contributor

JackBoosY commented Feb 14, 2022

x64_osx failed but I think it's not because of this PR:

duplicate symbol '_METIS_NodeND' in:
    lib/Graclus/libgraclus.a(ometis.c.o)
    /Users/vagrant/Data/installed/x64-osx/debug/lib/libmetis.a(ometis.c.o)
ld: 1 duplicate symbol for architecture x86_64

Yeah, that was resolved.

@vicroms
Copy link
Member

vicroms commented Feb 22, 2022

The team discussed this PR and the general approach to improve our Linux dynamic experience:
For the most part we like the changes in this PR but there are some things that we would like to see changed:

  • We would like this to work on more than just x64, this means finding a way to acquire patchelf for different platforms.
  • We would like this to be applied to each package built in the dynamic triplet automatically, this means proposing a way to hook up processes per triplet (we have a few suggestions):
    • Find a way to do this by setting VCPKG_CHAINLOAD_TOOLCHAIN_FILE as proposed by this comment.
    • Introduce a new triplet variable where we could hook post processing scripts, e.g.: VCPKG_POST_BUILD_SCRIPTS (name is a placeholder).
  • If this is to be a port function, we would like to move it out of scripts/cmake into a proper scripts port.

@Osyotr
Copy link
Contributor Author

Osyotr commented Feb 22, 2022

@vicroms

We would like this to work on more than just x64, this means finding a way to acquire patchelf for different platforms.

There are prebuilt binaries for many architectures: https://github.com/NixOS/patchelf/releases/tag/0.14.5, but should we consider Windows->Linux cross compilation scenario? I have no idea whether it's possible to build patchelf for Windows. Anyway, it may be done in later PR.

  • Find a way to do this by setting VCPKG_CHAINLOAD_TOOLCHAIN_FILE as proposed by this comment.

The comment you linked describes the way to automatically pass -Wl,-rpath,'$ORIGIN' flag to triplets. This would still require post-build patching because -Wl,-rpath,'$ORIGIN' -Wl,-rpath,'/path/on/host' will result in rpath $ORIGIN:/path/on/host which is undesired and would require patching ports. Also, many ports do not respect custom linker flags.

  • Introduce a new triplet variable where we could hook post processing scripts, e.g.: VCPKG_POST_BUILD_SCRIPTS (name is a placeholder).

This is what I was thinking about, too. So the whole x64-linux-dynamic triplet would look like this:

set(VCPKG_TARGET_ARCHITECTURE x64)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE dynamic)

set(VCPKG_CMAKE_SYSTEM_NAME Linux)

set(VCPKG_POST_BUILD_FIXUP_RPATH ON)

Same for BSD and other platforms that use elf format.

If this is to be a port function, we would like to move it out of scripts/cmake into a proper scripts port.
I would prefer to not make this function public. Would it be acceptable to move it into scripts/fixup_rpaths.cmake?

Also, is there any conclusion on what to do with tools (#17607)? This is something I didn't think of very well in my initial implementation. Having multiple combinations of tools paths increases complexity of this PR. Main issue is debug variant of tools.

@Neumann-A
Copy link
Contributor

If a port is adding rpaths like described:

-Wl,-rpath,'$ORIGIN' -Wl,-rpath,'/path/on/host'

this needs to be patch out any way. Ports are not allow to set rpaths, only the toolchain is.

Also, many ports do not respect custom linker flags.

this needs to be fixed. The toolchain file should define all flags and pass them on to the build.

@Osyotr
Copy link
Contributor Author

Osyotr commented Feb 23, 2022

@Neumann-A you are right, it's better to build with correct rpath than to fix afterwards. This would require writing a post-build check in vcpkg-tool and fixing affected ports one by one. Unfortunately, I don't have enough knowledge, experience and time to do that.
I also don't see why we can't have both solutions simultaneously. Once all ports and build systems are fixed, we can drop post-build fixup.

@Osyotr
Copy link
Contributor Author

Osyotr commented Feb 23, 2022

Also, without resolving #17607 it will be hard to set correct rpath for tools at build time.

@Neumann-A
Copy link
Contributor

correct rpath for tools at build time.

that will be only $ORIGIN and go the windows way. Either copy the *.so or symlink it into the folder. I think that is the only valid approach considering the circumstances.

@Osyotr
Copy link
Contributor Author

Osyotr commented Feb 23, 2022

Either copy the *.so or symlink it into the folder

Why? What's the difference between this and setting rpath to $ORIGIN/../lib?

@Neumann-A
Copy link
Contributor

Why? What's the difference between this and setting rpath to $ORIGIN/../lib?

It doesn't require looking into the parent if there is a /lib folder which does not exists if it is installed into tools/port/bin or tools/port/.
The Idea of doing $ORIGIN/../lib is that the folder structure is such that there is a bin folder and besides it is a lib folder. I personally don't mind it but see that it requires a fixed layout which vcpkg simply does not have. As such setting it to $ORIGIN and do it the windows way seems simply more correct and easier to implement.

@Osyotr
Copy link
Contributor Author

Osyotr commented Feb 23, 2022

It doesn't require looking into the parent if there is a /lib folder which does not exists if it is installed into tools/port/bin or tools/port/.

I'm already calculating relative path to lib/ in this PR and release tools do have right rpath ($ORIGIN/../../lib or $ORIGIN/../../../lib etc.)
The only issues are debug tools but I guess it can be fixed by checking whether a tool is located in debug/ subdirectory and if it is then use $ORIGIN/../~/../debug/lib. I don't think vcpkg should install debug tools, though.

@dg0yt
Copy link
Contributor

dg0yt commented Feb 24, 2022

I don't think vcpkg should install debug tools, though.

With the current ad-hoc layout, debug tools also need help to find data files. For Linux, it would be much easier if debug variants were installed into an entirely separate prefix.

@Neumann-A
Copy link
Contributor

Neumann-A commented Feb 25, 2022

Introduce a new triplet variable where we could hook post processing scripts,

hrhr. I have an idea: somebody tried cmake_language(DEFER) to inject post build stuff ? doesn't work in script mode

@Osyotr Osyotr mentioned this pull request May 22, 2022
@Curve Curve mentioned this pull request Jun 7, 2022
@PhoebeHui PhoebeHui changed the title Fixup rpath after building dynamic libraries on linux [vcpkg] Fixup rpath after building dynamic libraries on linux Jun 9, 2022
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jun 17, 2022
@JackBoosY
Copy link
Contributor

Tagged reviewed so other members will review this changes (doesn't means approved).

@dan-shaw dan-shaw removed the info:reviewed Pull Request changes follow basic guidelines label Jun 23, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Jul 14, 2022

For cross-compiling you can always use a static host triplet. Cross-compiling to mingw triplets works very well this way.

RPATH must not contain absolute paths. It breaks relocatability of cached artifacts.

@Jamlys
Copy link
Contributor

Jamlys commented Jul 14, 2022

For cross-compiling you can always use a static host triplet. Cross-compiling to mingw triplets works very well this way.

RPATH must not contain absolute paths. It breaks relocatability of cached artifacts.

Got it. Thx for suggestion. : )

@@ -4,3 +4,4 @@ set(VCPKG_LIBRARY_LINKAGE static)

set(VCPKG_CMAKE_SYSTEM_NAME Linux)

set(VCPKG_FIXUP_ELF_RPATH ON)
Copy link
Member

Choose a reason for hiding this comment

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

@Osyotr I think we're ready to merge this PR if this change is reverted.

We can always make this on-by-default and enable it in static triplet in a future change if we wanted to.

github-actions[bot]
github-actions bot previously approved these changes Jul 14, 2022
@vicroms vicroms added info:reviewed Pull Request changes follow basic guidelines and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jul 14, 2022
@vicroms vicroms merged commit 42b2876 into microsoft:master Jul 18, 2022
@Osyotr Osyotr deleted the linux_dynamic_rpath branch July 18, 2022 19:45
@JackBoosY
Copy link
Contributor

So currently the only thing preventing us from converting x64-linux-dynamic to an official triplet is our budget?

@BillyONeal
Copy link
Member

Yes

@silverqx
Copy link
Contributor

Great powerful PR, thx @Osyotr

@vserdyuk
Copy link
Contributor

Or at least this can be an optional approach

This is already optional. But there is really no need for it to be disabled.

personally I also hope that this solution can work on different platforms

In this PR I've added patchelf for x64 and aarch64, but this list is easily expandable. It's also available for armv7l, i686, ppc64le and s390x, but I've never heard of anyone using these platforms for development. If you are such person, please tell us more.

I am using an armv7l host (32-bit Ubuntu on Raspberry Pi 4B). Had to fix the script (HOST_ARCH is checked, but not initialized) and add the appropriate download hash. See PR for details: #26510

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants