Inject LD_LIBRARY_PATH library path into Python manifest install and setup

[magnetikonline]

This PR sets up the env var LD_LIBRARY_PATH for use when installing a fresh Python manifest and configuring it for use - ensures the Python manifest version installed can have it's libs correctly located.

Note this only applies to action runs under Linux based OSes.

While in the area, noted small typo in docs/contributors.md.

Related prior discussions:

• Unable to dynamically download and install manifests - library not found/path issue #131
• Adding Python manifest lib path to LD_LIBRARY_PATH before installing pip python-versions#49

[magnetikonline]

Here we go @maxim-lobanov / @MaksimZhukov - this does the job nicely.

[maxim-lobanov]

I could be wrong but I think it is better to always add pyLibPath to begin of the LD_LIBRARY_PATH because if pyLibPath in LD_LIBRARY_PATH but it is not the first one, it can cause incorrect work of Python.

[magnetikonline]

@maxim-lobanov the libs loaded (filename) are unique to the Python version. Yeah I was trying to avoid the situation if someone did decide to include the setup-python action multiple times for the same version - and duplication ends up being a problem?

Maybe it's better to:

• Split existing path.
• If found - remove item.
• Add item to start of list.
• join() list - as the new mutation of LD_LIBRARY_PATH?

What do you think @maxim-lobanov ?

[maxim-lobanov]

Even someone decide to include setup-python action multiple times for the same version (very strange case :) ), duplication won't cause problems.

If we would like to avoid it, your current approach should work (since the libs loaded (filename) are unique to the Python version).

cc: @konradpabjan

[magnetikonline]

Cheers - yeah let's see what @konradpabjan thinks as a second opinion.

Able to pivot this to above if needed (re-insert as the first component).

[konradpabjan]

@magnetikonline

Overall the current approach seems good 👍

I do have once concern though around where where the env variable is set:
core.exportVariable('LD_LIBRARY_PATH', pyLibPath + libPath);

We're currently omitting a : between the values (this works if libpath is empty which I think is the case with our hosted environments). When this value is not empty, things could break.

I think core.exportVariable('LD_LIBRARY_PATH', pyLibPath + ':' + libPath); or even core.exportVariable(pylibpath.concat(':', libPath); is needed.

The order of pyLibPath and libPath doesn't really make a difference since they're unique. No preference with regards to that.

[magnetikonline]

@konradpabjan this is covered (the :):

const libPath = process.env.LD_LIBRARY_PATH
      ? `:${process.env.LD_LIBRARY_PATH}`
      : '';

Note if process.env.LD_LIBRARY_PATH does exist libPath is set to : plus the current library path. Make sense?

[konradpabjan]

Got it! Thanks for the clarification. I accidently missed it earlier 😞

With everything looks great!

[magnetikonline]

Not a problem - it's an easy miss 😄

[magnetikonline]

Thanks @maxim-lobanov 👍

I had a bit more of a think about this - if we do always want the current Python lib path first - and never duplicated, this would be the implement. Nothing too tricky, should we swap this out?

core.exportVariable('pythonLocation', installDir);

if (IS_LINUX) {
  const libPaths = (process.env.LD_LIBRARY_PATH || '').split(':');
  const pyLibPath = path.join(installDir, 'lib');

  const foundIdx = libPaths.indexOf(pyLibPath);
  if (foundIdx > -1) {
    libPaths.splice(foundIdx, 1);
  }

  libPaths.unshift(pyLibPath);
  core.exportVariable('LD_LIBRARY_PATH', libPaths.join(':'));
}

core.addPath(installDir);
core.addPath(binDir(installDir));

Maybe this is a little more reliable - the most recent Python lib path will always be first this way.

[maxim-lobanov]

@magnetikonline , have you tested that your changes work as expected for you?
You can call your fork in workflow just for testing purpose

- uses: magnetikonline/setup-python@inject-ld_library_path
  with:
    python-version: '3.x'

[magnetikonline]

@magnetikonline , have you tested that your changes work as expected for you?
You can call your fork in workflow just for testing purpose

I have @maxim-lobanov - tested last week. 👍

[konradpabjan]

Thanks for the PR @magnetikonline 🎉

I'm going to go ahead and approve this and merge it into the main branch. I don't expect any problems but this will allow us to test on a large subset of users that pin to @main. If everything is good, I'll create a new release and update the v2 tag in a few days.

[magnetikonline]

Awesome - thanks for your time @konradpabjan / @maxim-lobanov - I just retested setup-python@main vs. setup-python@v2 and yes - it's now 100% when using main.

Will be great to see this in V2 soon! 👍

[magnetikonline]

Hello @konradpabjan - just wondering if we have an ETA for tagging v2 with this PR/commit?

Thanks!

[konradpabjan]

@magnetikonline I've created a new release (v2.1.4) and I've updated the v2 tag with the latest changes.

Thanks again for the PR!

[magnetikonline]

Fantastic - thanks @konradpabjan 👍