Skip to content

Fix guile environment variable on linux #551

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MorganJamesSmith
Copy link
Contributor

@MorganJamesSmith MorganJamesSmith commented Oct 17, 2023

The commit message explains more.

Part of the issue here though is that we have slightly different logic when running in the build directory and when running when installed. Other projects (GNU Guix, mumi, etc...) get around this by defining a "pre-inst-env" shell script that sets environment variables.

So for example we could have a shell script that looks like this (untested):

#!/bin/sh

# TODO: python environment variables
# TODO: system library environment variables

build_dir="$(pwd)"
bin_dir="$(build_dir)/studio"
guile_dir="$(build_dir)/libfive/bind/guile"

PATH="$bin_dir${PATH:+:}$PATH"
GUILE_LOAD_PATH="$guile_dir${GUILE_LOAD_PATH:+:}$GUILE_LOAD_PATH"
GUILE_LOAD_COMPILED_PATH="$guile_dir${GUILE_LOAD_COMPILED_PATH:+:}$GUILE_LOAD_COMPILED_PATH"
export PATH GUILE_LOAD_PATH GUILE_LOAD_COMPILED_PATH

Then to run the program out of the build directory the command would be "./pre-inst-env Studio".

This would simplify the following things:

We could remove the guile_load_path and guile_load_compiled_path stuff from these parts of the code:

./libfive/test/guile.cpp:36
./studio/src/guile/interpreter.cpp:86
./studio/src/guile/interpreter.cpp:94
./doc/guide.md:207

We could remove the python load path stuff from here (I think. Not sure how python environment variable stuff works):

./studio/src/python/interpreter.cpp:214

We could add the library binary to system paths which would simplify:

./libfive/bind/guile/libfive/lib.scm:30 to 61
./libfive/bind/python/libfive/ffi.py:37:
./studio/src/app.cpp:32

Let me know what you think. I'm not really sure how to integrate this pre-inst-env idea with cmake stuff so any help on that front would be nice.

Despite the adjacent comment, the original code does not fall back to system
libraries as the environment variable got completely overridden.  This change
aligns the code with the comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant