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.

Sign up for GitHub

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

Jump to bottom

Inject LD_LIBRARY_PATH library path into Python manifest install and setup #144

Merged
merged 3 commits into from
Oct 5, 2020
Merged

Inject LD_LIBRARY_PATH library path into Python manifest install and setup #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2513d89
Adding LD_LIBRARY_PATH env var to both setup and install tasks
magnetikonline Oct 1, 2020
497269a
Rebuild dist/index.js
magnetikonline Oct 1, 2020
f2b6cd4
Fixed some typos in contributors.md Markdown
magnetikonline Oct 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions dist/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6422,6 +6422,7 @@ var __importStar = (this && this.__importStar) || function (mod) {
return result;
};
Object.defineProperty(exports, "__esModule", { value: true });
const path = __importStar(__webpack_require__(622));
const core = __importStar(__webpack_require__(470));
const tc = __importStar(__webpack_require__(533));
const exec = __importStar(__webpack_require__(986));
Expand All @@ -6432,6 +6433,7 @@ const MANIFEST_REPO_NAME = 'python-versions';
const MANIFEST_REPO_BRANCH = 'main';
exports.MANIFEST_URL = `https://raw.githubusercontent.com/${MANIFEST_REPO_OWNER}/${MANIFEST_REPO_NAME}/${MANIFEST_REPO_BRANCH}/versions-manifest.json`;
const IS_WINDOWS = process.platform === 'win32';
const IS_LINUX = process.platform === 'linux';
function findReleaseFromManifest(semanticVersionSpec, architecture) {
return __awaiter(this, void 0, void 0, function* () {
const manifest = yield tc.getManifestFromRepo(MANIFEST_REPO_OWNER, MANIFEST_REPO_NAME, AUTH, MANIFEST_REPO_BRANCH);
Expand All @@ -6443,6 +6445,7 @@ function installPython(workingDirectory) {
return __awaiter(this, void 0, void 0, function* () {
const options = {
cwd: workingDirectory,
env: Object.assign(Object.assign({}, process.env), IS_LINUX && { 'LD_LIBRARY_PATH': path.join(workingDirectory, 'lib') }),
silent: true,
listeners: {
stdout: (data) => {
Expand Down Expand Up @@ -6688,6 +6691,7 @@ const installer = __importStar(__webpack_require__(824));
const core = __importStar(__webpack_require__(470));
const tc = __importStar(__webpack_require__(533));
const IS_WINDOWS = process.platform === 'win32';
const IS_LINUX = process.platform === 'linux';
// Python has "scripts" or "bin" directories where command-line tools that come with packages are installed.
// This is where pip is, along with anything that pip installs.
// There is a seperate directory for `pip install --user`.
Expand Down Expand Up @@ -6760,6 +6764,13 @@ function useCpythonVersion(version, architecture) {
].join(os.EOL));
}
core.exportVariable('pythonLocation', installDir);
if (IS_LINUX) {
const libPath = (process.env.LD_LIBRARY_PATH) ? `:${process.env.LD_LIBRARY_PATH}` : '';
const pyLibPath = path.join(installDir, 'lib');
if (!libPath.split(':').includes(pyLibPath)) {
core.exportVariable('LD_LIBRARY_PATH', pyLibPath + libPath);
}
}
core.addPath(installDir);
core.addPath(binDir(installDir));
if (IS_WINDOWS) {
Expand Down
6 changes: 4 additions & 2 deletions docs/contributors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ In order to avoid uploading `node_modules/` to the repository, we use [vercel/nc
### Developing

If you're developing locally, you can run
```

```sh
npm install
tsc
ncc build src/setup-python.ts
```
Any files generated using `tsc` will be added to `lib/`, however those files also are not uploaded to the repository and are exluded using `.gitignore`.

Any files generated using `tsc` will be added to `lib/`, however those files also are not uploaded to the repository and are excluded using `.gitignore`.

During the commit step, Husky will take care of formatting all files with [Prettier](https://github.com/prettier/prettier) (to run manually, use `npm run format`).

Expand Down
13 changes: 13 additions & 0 deletions src/find-python.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as core from '@actions/core';
import * as tc from '@actions/tool-cache';

const IS_WINDOWS = process.platform === 'win32';
const IS_LINUX = process.platform === 'linux';

// Python has "scripts" or "bin" directories where command-line tools that come with packages are installed.
// This is where pip is, along with anything that pip installs.
Expand Down Expand Up @@ -109,6 +110,18 @@ async function useCpythonVersion(
}

core.exportVariable('pythonLocation', installDir);

if (IS_LINUX) {
const libPath = process.env.LD_LIBRARY_PATH
? `:${process.env.LD_LIBRARY_PATH}`
: '';
const pyLibPath = path.join(installDir, 'lib');

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

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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!

Copy link
Contributor Author

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 😄

core.exportVariable('LD_LIBRARY_PATH', pyLibPath + libPath);
}
}

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

Expand Down
5 changes: 5 additions & 0 deletions src/install-python.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const MANIFEST_REPO_BRANCH = 'main';
export const MANIFEST_URL = `https://raw.githubusercontent.com/${MANIFEST_REPO_OWNER}/${MANIFEST_REPO_NAME}/${MANIFEST_REPO_BRANCH}/versions-manifest.json`;

const IS_WINDOWS = process.platform === 'win32';
const IS_LINUX = process.platform === 'linux';

export async function findReleaseFromManifest(
semanticVersionSpec: string,
Expand All @@ -35,6 +36,10 @@ export async function findReleaseFromManifest(
async function installPython(workingDirectory: string) {
const options: ExecOptions = {
cwd: workingDirectory,
env: {
...process.env,
...(IS_LINUX && {LD_LIBRARY_PATH: path.join(workingDirectory, 'lib')})
magnetikonline marked this conversation as resolved.
Show resolved Hide resolved
},
silent: true,
listeners: {
stdout: (data: Buffer) => {
Expand Down