Trim LD_LIBRARY_PATH on startup #1740
Conversation
This way setting --data-dir and --extension-dir in the config file will work for --install--extension and whatnot.
src/node/entry.ts
Outdated
| // Removes the leading path added by ./ci/build/code-server.sh to use our bundled | ||
| // dynamic libraries. See ci/build/build-standalone-release.sh | ||
| // This is required to avoid child processes using our bundled libraries. | ||
| process.env[ldVar] = process.env[ldVar]?.replace(path.dirname(process.execPath) + ":", "") |
There was a problem hiding this comment.
I am not sure that LD_LIBRARY_PATH is populated at this point so it might be empty. You can confirm correct operation if you run the following from a child process:
ldd $(which cmake) | grep code-server
if you see any output it is still pointing at code server then it might not have taken. I think you might need to do something like AppImage where they rewrite the linker path for the binary to their library paths, that way you will not pollute downstream. I am not sure if this https://medium.com/@nehckl0/creating-relocatable-linux-executables-by-setting-rpath-with-origin-45de573a2e98 is feasible. That might be the best hope since you can put that in the installer when the location is selected.
There was a problem hiding this comment.
I built off this PR and can confirm there's no output for that command whereas there was before.
src/node/entry.ts
Outdated
| @@ -126,19 +122,40 @@ const main = async (cliArgs: Args): Promise<void> => { | |||
| } | |||
| } | |||
|
|
|||
| function trimLDLibraryPath(): void { | |||
| let ldVar: string | |||
| if (process.platform === "linux") { | |||
There was a problem hiding this comment.
I think we might want to check if the variable exists or something rather than use the platform because process.platform returns non-linux values even if uname normally returns Linux. https://nodejs.org/api/process.html#process_process_platform
There was a problem hiding this comment.
Actually that might only be the case for Android. 🤔 The rest seem to match up in terms of uname vs process.platform.
- Instead we now use CentOS 7 for the static build to guarantee that we only depend on libc v2.17 - For macOS we now pull in a static node binary and bundle that instead.
|
Closing as this isn't going to work and be a good solution. There's no good way to ensure every time the node binary is invoked, we have our The plan is to use a static binary on macOS and switch to building the static release on CentOS which should be enough to point everyone else to just use the npm package if they're on something older. |
Not sure if this will work or we have to somehow set these variables
every time the bundled node binary is ran and then trim them on startup.
Updates #1738