GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Inject LD_LIBRARY_PATH library path into Python manifest install and setup by magnetikonline · Pull Request #144 · actions/setup-python

Conversation

magnetikonline commented Oct 1, 2020

Copy link

Copy Markdown

Contributor

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

magnetikonline added 3 commits

October 1, 2020 15:38


          Adding LD_LIBRARY_PATH env var to both setup and install tasks

2513d89


          Rebuild dist/index.js

497269a


          Fixed some typos in contributors.md Markdown

f2b6cd4

magnetikonline commented Oct 1, 2020

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

magnetikonline mentioned this pull request

Oct 1, 2020

Adding Python manifest lib path to LD_LIBRARY_PATH before installing pip actions/python-versions#49

Closed

maxim-lobanov reviewed

Oct 1, 2020

View reviewed changes

Comment thread

src/find-python.ts

		: '';
		const pyLibPath = path.join(installDir, 'lib');

		if (!libPath.split(':').includes(pyLibPath)) {

maxim-lobanov Oct 1, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

magnetikonline Oct 1, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

maxim-lobanov Oct 1, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

magnetikonline Oct 1, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

konradpabjan Oct 5, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

magnetikonline Oct 5, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

konradpabjan Oct 5, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

With everything looks great!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

magnetikonline reacted with thumbs up emoji

All reactions

👍 1 reaction

magnetikonline Oct 5, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not a problem - it's an easy miss 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

maxim-lobanov reviewed

Oct 1, 2020

View reviewed changes

Comment thread

src/install-python.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

maxim-lobanov approved these changes

Oct 1, 2020

View reviewed changes

magnetikonline commented Oct 2, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

maxim-lobanov commented Oct 5, 2020

Copy link

Copy Markdown

Contributor

@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'

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

magnetikonline commented Oct 5, 2020

Copy link

Copy Markdown

Contributor Author

@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. 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

konradpabjan approved these changes

Oct 5, 2020

View reviewed changes

konradpabjan left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

maxim-lobanov reacted with hooray emoji

maxim-lobanov reacted with rocket emoji

All reactions

🎉 1 reaction
🚀 1 reaction

konradpabjan merged commit 878156f into actions:main

Oct 5, 2020

magnetikonline commented Oct 5, 2020

Copy link

Copy Markdown

Contributor Author

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! 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thomwiggers mentioned this pull request

Oct 6, 2020

Use Github Actions to run tests (+ many SPHINCS+ AVX2 fixes for Windows) PQClean/PQClean#300

Merged

10 tasks done

magnetikonline commented Oct 12, 2020

Copy link

Copy Markdown

Contributor Author

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

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

konradpabjan commented Oct 14, 2020

Copy link

Copy Markdown

Contributor

@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 reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

magnetikonline commented Oct 14, 2020

Copy link

Copy Markdown

Contributor Author

Fantastic - thanks @konradpabjan 👍

konradpabjan reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

polm mentioned this pull request

Nov 4, 2020

Modifying LD_LIBRARY_PATH causes issues #159

Closed

2 of 7 tasks

RalfG mentioned this pull request

Nov 19, 2020

Temporary workaround for LD_LIBRARY_PATH issue (#26) RalfG/python-wheels-manylinux-build#28

Merged

yoichiwo7 mentioned this pull request

Jan 22, 2021

v2 tag doesn't use the latest 2.x.x version #183

Closed

5 of 8 tasks

tdfacer pushed a commit to ifit/setup-python that referenced this pull request

Oct 7, 2025


          Inject LD_LIBRARY_PATH library path into Python manifest install and …

081896c

…setup (actions#144)

* Adding LD_LIBRARY_PATH env var to both setup and install tasks

* Rebuild dist/index.js

* Fixed some typos in contributors.md Markdown

Reviewers

maxim-lobanov

maxim-lobanov approved these changes

+1 more reviewer

konradpabjan

konradpabjan approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

magnetikonline commented Oct 1, 2020

Uh oh!

magnetikonline commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

magnetikonline commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxim-lobanov commented Oct 5, 2020

Uh oh!

magnetikonline commented Oct 5, 2020

Uh oh!

konradpabjan left a comment

Choose a reason for hiding this comment

Uh oh!

magnetikonline commented Oct 5, 2020

Uh oh!

magnetikonline commented Oct 12, 2020

Uh oh!

konradpabjan commented Oct 14, 2020

Uh oh!

magnetikonline commented Oct 14, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

magnetikonline commented Oct 2, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.