Add support for GLJPATH environment variable #89
Conversation
To avoid adding things that are often lying around... The .clj and .glj are to ignore top level scripts for testing stuff. The GNUmakefile and .cache are for alternate Makefile stuff like I do in glojurelang#75 If that doesn't get merged I can just symlink to it since GNUmakefile has precedence over Makefile (for GNU make).
jfhamlin
left a comment
jfhamlin
left a comment
There was a problem hiding this comment.
thanks for adding the GLJPATH support! left a few comments, mainly on command line parsing and the command line interface
test/glojure/test_glojure/cli.glj
Outdated
| (let [lib1-dir (str (os.Getenv "PWD") "/test_gljpath_priority1") | ||
| lib2-dir (str (os.Getenv "PWD") "/test_gljpath_priority2") | ||
| lib3-dir (str (os.Getenv "PWD") "/test_gljpath_priority3") |
There was a problem hiding this comment.
love that we're testing the edge case here! re test directory, i think it'd be preferable to use a temp directory rather than the working directory. os.TempDir will return the path to use for temporary files on the current platform
| // parseArgs parses command line arguments and returns an Args struct | ||
| func parseArgs(args []string) (*Args, error) { |
There was a problem hiding this comment.
i should have called this out in the prior command line flag PR, but as this is getting more involved, let's use the standard library's flag parsing: https://pkg.go.dev/flag
pkg/gljmain/gljmain.go
Outdated
| // Process -I include paths last (add to front of load path, highest priority) | ||
| // Process in reverse order so first -I on command line gets highest priority |
There was a problem hiding this comment.
this seems duplicative with GLJPATH, and "include" evokes C/C++ — i'm not sure the -I command line flag makes sense for clojure
| for i := len(paths) - 1; i >= 0; i-- { | ||
| path := paths[i] | ||
| if path != "" { | ||
| // Skip non-existent path directories | ||
| if _, err := os.Stat(path); err == nil { | ||
| runtime.AddLoadPath(os.DirFS(path), true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
could we could drop the true second arg and start iterating from index 0 to get the same result?
There was a problem hiding this comment.
This all makes sense. I'll update in the morning.
There was a problem hiding this comment.
We probably want . at the very end of the loadpath (which is what you had).
It seems like there is 1 other entry in the loadpath when we start adding things here, but I was unable to print its value.
The other things need to be added to the front of the loadpath as I'm doing.
I'll refactor to make 2 funcs: AddLoadPath (same as original) and PrependLoadPath.
$ GLJPATH=foo:bar ./bin/linux_amd64/glj -e '*glojure-load-path*'
("foo" "bar" "<StdLib>" ".")
|
I added a commit to support a Seems useful: c3eb307 |
|
Closing this and revisiting the concept in #123 |
2 participants
For runtime loading of
.gljfiles.Note: this PR includes #88 commits so that one should probably be sorted first.
I can adjust this one to any changes you want there.