428068 - Dehydra/Treehydra: enhanced include path control

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[David Mandelin [:dmandelin]]

This isn't that important yet, although I do have some use for it already. Take this as a question/discussion item for now.

It would be nice to be able to add to the include path. E.g., I have my library of general analysis scripts, and it would be nice to be able to refer to it more easily from other scripts. Currently the include path is fixed to cwd, then Dehydra plugin dir.

I'm not sure what API is good for this. A few possibilities are (1) more stuff in plugin-arg (which would be getting crowded), (2) environment variable (traditional, workable, but faintly icky), and (3) a config file in the cwd (.dehydrarc or something). There could also be a builtin function for modifying it (c.f. modifying sys.path in Python), but that's just for quick hackery and not a full solution to the problem.

Comment 1

[(dormant account)]

Ooh, config file sounds excellent.
perhaps we could make this a plugin-level affair? Then it could specify plugin dir too.

/etc/gcc-plugin.conf
and ~/.gcc-plugin.conf or something would be cool

Comment 2

[David Mandelin [:dmandelin]]

There's a specific need here wrt bug 420933. When running xhydra as part of a build, the current directory could be anything. My initial attempt to run outparams.js as part of a build failed because xpcom/analysis isn't the current directory. 

One plausible fix is to add the directory containing the script specified on the command line to the include path. Do you foresee any problems with that approach?

Other than that, in order for this to work reliably in the build process, I think it would have to be specified in the command line. I have a pending patch that allows xhydra access to the -fplugin-arg command-line arg. But that strikes me as kind of painful for that application. Can we just add -fplugin-path or something?

Comment 3

[(dormant account)]

(In reply to comment #2)
> There's a specific need here wrt bug 420933. When running xhydra as part of a
> build, the current directory could be anything. My initial attempt to run
> outparams.js as part of a build failed because xpcom/analysis isn't the current
> directory. 
> 
> One plausible fix is to add the directory containing the script specified on
> the command line to the include path. Do you foresee any problems with that
> approach?

Sounds good to me.
> 
> Other than that, in order for this to work reliably in the build process, I
> think it would have to be specified in the command line. I have a pending patch
> that allows xhydra access to the -fplugin-arg command-line arg. But that
> strikes me as kind of painful for that application. Can we just add
> -fplugin-path or something?

I'd rather not add arguments to gcc.
> 

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch for partial spec: blocking part only

This patch takes care of the important stuff for bug 420933:

- Directory containing toplevel script is added to include path.

- Now that the include path can be multiple directories, it is managed as a JS array, (toplevel this).include_path. This gives scripts the ability to change the path, a la Python sys.path. I don't really need this, the main reason was that I needed some kind of list of paths, and this is the most flexible way.

- Refactored Include/dehydra_loadScript implementation because the temporary replacement of this->globalObj removes access to the top-level object, which the script loader needs in order to access the search path. The namespace is now a parameter of loadScript, which is only called in this one place anyway.

The only thing not done is the config file, which doesn't block 420933.

Comment 5

[(dormant account)]

Comment on attachment 316889 [details] [diff] [review]
Patch for partial spec: blocking part only

>Bug 428068 Partial: add script dir to include path
>
>diff -r 5fde676303a5 dehydra.c
>--- a/dehydra.c	Thu Apr 17 14:36:31 2008 -0700
>+++ b/dehydra.c	Mon Apr 21 14:55:55 2008 -0700
>@@ -2,6 +2,7 @@
> #include <jsapi.h>
> #include <unistd.h>
> #include <stdio.h>
>+#include <libgen.h>
> 
> #include <config.h>
> #include <system.h>
>@@ -42,6 +43,8 @@ static const char *STATIC = "isStatic";
> static const char *STATIC = "isStatic";
> static const char *VIRTUAL = "isVirtual";
> 
>+static const char *INCLUDE_PATH = "include_path";
>+
> #ifdef JS_GC_ZEAL
> static JSBool
> GCZeal(JSContext *cx, uintN argc, jsval *vp)
>@@ -78,15 +81,6 @@ void dehydra_init(Dehydra *this, const c
> #endif
>     {0}
>   };
>-
>-  char *c = strrchr(file, '/');
>-  if (c) {
>-    // include / in the copy
>-    int len = c - file + 1;
>-    this->dir = xmalloc(len + 1);
>-    strncpy(this->dir, file, len);
>-    this->dir[len] = 0;
>-  }
> 
>   this->fndeclMap = pointer_map_create ();
>   this->rt = JS_NewRuntime (0x9000000L);
>@@ -112,6 +106,17 @@ void dehydra_init(Dehydra *this, const c
>                     OBJECT_TO_JSVAL (this->rootedFreeArray),
>                     NULL, NULL, JSPROP_ENUMERATE);
>   JS_SetVersion (this->cx, (JSVersion) 170);
>+
>+
>+  /* Initialize include path. */
>+  JSObject *path = JS_NewArrayObject(this->cx, 0, NULL);
>+  xassert(path);
>+  JS_DefineProperty(this->cx, this->globalObj, INCLUDE_PATH, 
>+                    OBJECT_TO_JSVAL(path), NULL, NULL, 
>+                    JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);

Please don't bother checking allocations or for failure. Rest of *hydra doesn't care, so this shouldn't. Don't use JS_DefineProperty. This code should use dehydra_defineArrayProperty on this->globalObj(this happens in a bunch of other places in this patch). 

Other than that, I think it's ok. Might want to consider namespacing this bonus info. Have a ._env(or maybe _sys is better?) object which would include the included Array and the include path.

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Revised path

Per previous comment.

I didn't move _includedPath to sys because there isn't just one _includedPath, there is actually one per namespace created by include().

Comment 7

[David Mandelin [:dmandelin]]

Attachment: Fixed some bitrot

Comment 8

[(dormant account)]

Comment on attachment 317617 [details] [diff] [review]
Fixed some bitrot

Looks good, my only comment is that the testcase just tests include(), it would've worked fine in the previous dehydra version. So I don't see a real need for another test.

Comment 9

[David Mandelin [:dmandelin]]

(In reply to comment #8)
> (From update of attachment 317617 [details] [diff] [review])
> Looks good, my only comment is that the testcase just tests include(), it
> would've worked fine in the previous dehydra version. So I don't see a real
> need for another test.

Pushed.

Rest assured, the test case most definitely fails in the previous version. :-P The key is that the script and included file are not in the cwd.