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

[JK] Portable/Relocatable Python Linking #275

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaswanthikolla
Copy link

@jaswanthikolla jaswanthikolla commented May 19, 2024

What's the Issue?
Python binary is compiled with rpath that's /opt/hostedtoolcache/Python. Now, latest github runners define RUNNER_TOOL_CACHE/AGENT_TOOLSDIRECTORY differently than /opt/hostedtoolcache and that installs python at /home/runner/_work/_tool/Python/. So, with this, there are 2 issues.

  1. sudo python --version doesn't work ( See Error section) because most systems's doesn't allow passing LD_LIBRARY_PATH due to security issues.
  2. python --version doesn't work without setting environment variable LD_LIBRARY_PATH

output of ldd :

runner@arss-runner-xxxx-runner-5jvhz:~/_work/_tool/Python/3.12.3/x64/bin$ ldd python3
	linux-vdso.so.1 (0x00007ffceb776000)
	libpython3.12.so.1.0 => not found
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f7ccc75c000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f7ccc991000)

Error:

python: error while loading shared libraries: libpython3.9.so.1.0: 
cannot open shared object file: No such file or directory

Solution:

$ORIGIN is used to as reference to the binary path. So, we can use rpath that references relative path instead of absolute compile time path. With this relative path, Python binaries become relocatable.

Change:

How this is tested?

  • I've also written a dockerfile to test this solution. Dockerfile moves the python installation from one location to another, and verifies it.
  • New Unit test / PR checks added.

Why bother about this issue?
LD_LIBRARY_PATH can't be used with sudo python due to security concerns. But, more details are here actions/setup-python#871

Comment on lines +97 to +120

It "Relocatable Python" {
$semversion = [semver] $Version
$pyfilename = "python$($semversion.Major).$($semversion.Minor)"
$artifactPath = Join-Path "Python" $Version | Join-Path -ChildPath $Architecture

$relocatedPython = Join-Path $HOME "relocated_python"
$relocatedPythonTool = Join-Path -Path $relocatedPython -ChildPath $artifactPath
$relocatedFullPath = Join-Path $relocatedPythonTool "bin" | Join-Path -ChildPath $pyfilename

# copy the current build to relocated_python
$toolCacheArtifact = Join-Path $env:RUNNER_TOOL_CACHE $artifactPath
moveAssets -source $toolCacheArtifact -destination $relocatedPythonTool
try {
# Verify that relocated Python works
$relocatedFullPath | Should -Exist
"$relocatedFullPath --version" | Should -ReturnZeroExitCode
"sudo $relocatedFullPath --version" | Should -ReturnZeroExitCode
}
finally {
# Revert the changes for other tests
moveAssets -source $relocatedPythonTool -destination $toolCacheArtifact
}
}
Copy link
Contributor

@mayeut mayeut Jun 1, 2024

Choose a reason for hiding this comment

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

I think this might fail on macOS.
@jaswanthikolla, can you run the full build in your fork to make sure this passes ?
It might save some time if it requires some modifications before maintainers validate the workflow / review the PR.

It might only fail when using a version not using the universal2 installer so worth checking a build of python < 3.11

Copy link
Author

Choose a reason for hiding this comment

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

This is Ubuntu/Linux Specific code, So this test doesn't get executed for macOS. May be, I can simplify by removing powershell code and write bash instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does fail on macOS which is an UNIX (it's UNIX specific code, not Linux): https://github.com/mayeut/python-versions/actions/runs/11766830107/job/32774981352

Copy link

@klalumiere klalumiere left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

@navarro967 navarro967 left a comment

Choose a reason for hiding this comment

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

Builds working on my forks thanks to this PR.

@jaswanthikolla
Copy link
Author

jaswanthikolla commented Oct 31, 2024

Any update on this PR? Can it be reviewed and merged?

@WTPOptAxe
Copy link

Just adding a +1 here. This is a real issue for many users, @jaswanthikolla has put in the work to resolve it for many use cases, and it's been going stale for 5 months.

@stolson
Copy link

stolson commented Nov 21, 2024

Also echoing the need for this fix to be merged.

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.

8 participants