-
Notifications
You must be signed in to change notification settings - Fork 0
Expose runtime dependencies of Nix package properly #22
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
base: main
Are you sure you want to change the base?
Conversation
SummaryAdd runtime dependency environment variables for Nix packages. Automatically sets up LD_LIBRARY_PATH, DYLD_LIBRARY_PATH, and PKG_CONFIG_PATH by querying nix-store for dependencies and scanning for lib/ and pkgconfig/ directories. Walkthrough
|
a66a411 to
3066501
Compare
0611ad0 to
04966c7
Compare
ofalvai
left a comment
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.
This is an AI-generated review. Please review it carefully.
Actionable comments posted: 1
lib/runtime_deps.lua
Outdated
|
|
||
| for _, lib_path in ipairs(lib_paths) do | ||
| local is_linux_glibc = os_type == "linux" and lib_path:match("glibc") | ||
| local is_darwin_llvm = os_type == "darwin" and lib_path:gmatch("(llvm)?(clang)?") |
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.
🐛 Bug
The gmatch method returns an iterator function, not a boolean. This will always evaluate to true (a function object is truthy), causing the condition to incorrectly filter out all Darwin library paths. Should use match instead to check if the pattern exists in the string.
🔄 Suggestion:
| local is_darwin_llvm = os_type == "darwin" and lib_path:gmatch("(llvm)?(clang)?") | |
| local is_darwin_llvm = os_type == "darwin" and lib_path:match("(llvm)|(clang)") |
daeb776 to
d5ab89e
Compare
ofalvai
left a comment
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.
This is an AI-generated review. Please review it carefully.
Actionable comments posted: 1
lib/runtime_deps.lua
Outdated
| -- for _, var in ipairs(lib_env_vars) do | ||
| -- table.insert(env_vars, var) | ||
| -- end |
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.
🐛 Bug
Library environment variables are computed but commented out, so LD_LIBRARY_PATH/DYLD_LIBRARY_PATH are never set. This means shared libraries from runtime dependencies won't be found at runtime, defeating the purpose of this feature. The library paths are found and filtered correctly, but they're never exposed to the environment.
🤖 Prompt for AI Agents:
In lib/runtime_deps.lua between lines 31 and 33, Uncomment the code that inserts library environment variables into the env_vars table so that LD_LIBRARY_PATH (Linux) and DYLD_LIBRARY_PATH (macOS) are properly set with the discovered library paths.
d5ab89e to
8fe209f
Compare
ofalvai
left a comment
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.
This is an AI-generated review. Please review it carefully.
Actionable comments posted: 1
lib/runtime_deps.lua
Outdated
| -- for _, var in ipairs(lib_env_vars) do | ||
| -- table.insert(env_vars, var) | ||
| -- end |
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.
🐛 Bug
Library environment variables are commented out, preventing LD_LIBRARY_PATH (Linux) and DYLD_LIBRARY_PATH (macOS) from being set. This means shared libraries from Nix dependencies won't be discoverable at runtime, breaking tools that need to load dynamic libraries.
🔄 Suggestion:
| -- for _, var in ipairs(lib_env_vars) do | |
| -- table.insert(env_vars, var) | |
| -- end | |
| for _, var in ipairs(lib_env_vars) do | |
| table.insert(env_vars, var) | |
| end |
ofalvai
left a comment
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.
This is an AI-generated review. Please review it carefully.
Actionable comments posted: 1
| { key = "PATH", value = bin_path }, | ||
| } | ||
|
|
||
| local runtime_deps = require("lib.runtime_deps") |
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.
🐛 Bug
The runtime_deps module has a critical bug where library environment variables (LD_LIBRARY_PATH/DYLD_LIBRARY_PATH) are commented out in lib/runtime_deps.lua at lines 31-33. This means the get_env_vars() function will only return PKG_CONFIG_PATH variables, not the library paths needed for runtime dependencies to work properly. The library environment variables must be uncommented to expose shared libraries at runtime.
🤖 Prompt for AI Agents:
In hooks/backend_exec_env.lua at line 17, Uncomment lines 31-33 in lib/runtime_deps.lua to enable\nLD_LIBRARY_PATH/DYLD_LIBRARY_PATH environment variables\nso that runtime dependencies are properly exposed.
98325e5 to
b31670f
Compare
e89c232 to
8add043
Compare
8add043 to
8516ce4
Compare
Add runtime dependency environment variables for Nix packages
Improve compatibility for tools by automatically setting up environment variables for their runtime dependencies. For example,
nixpkgs#rubyhave the following runtime dependencies:These store paths each have the usual
/lib,/includeand/lib/pkgsconfigdirs for shared C headers, dynamic libraries and pkgconfig definitions. These should be made available at runtime. For example, thegem installcommand often builds native C extensions of gems and needs to have access tolibyaml's C headers and.so/.dylib.How it works
When a tool is installed via the Nix backend, the plugin now:
nix-store --requisitesto get all runtime dependencieslib/andlib/pkgconfig/directoriesLD_LIBRARY_PATH(Linux) orDYLD_LIBRARY_PATH(macOS) for shared librariesPKG_CONFIG_PATHfor pkg-config filesThis ensures that Nix-installed tools can find their dependencies at runtime without requiring additional configuration.