Re: Another package manager issue

[John W. Eaton]

On 29-Aug-2006, I wrote:

| On 29-Aug-2006, David Bateman wrote:
| 
| | John W. Eaton wrote:
| | > On 28-Aug-2006, David Bateman wrote:
| | >
| | > | I see two solutions. Mark all of the
| | > | sub-functions that call addpath in pkg.m as commands,
| | >
| | > I don't understand how this helps.
| | >   
| | Think about addpath which is a function marked as a command. When it
| | adds a path with a PKG_ADD file in it is is evaluated correctly. I
| | haven't looked at the code, but I suspect that if a function marked as a
| | command is run, the level is not incremented. Therefore if all of the
| | functions up to and including the function with the addpath are marked
| | as commands, then addpath will appear at the top-level.
| 
| It's not that the addpath function is marked as a command, it is that
| it is a built-in function.  Octave considers itself at the top level
| if curr_sym_tab == top_level_sym_tab (see at_top_level in
| variables.cc).  We don't reset curr_sym_tab when a built-in function
| executes because a built-in function has no local symbol table.
| 
| Should built-in functions execute so that at_top_level is true?  If
| not, we could always set curr_sym_tab to 0 while built-in functions
| execute.
| 
| | > | or use evalin to
| | > | run the addpath command in the top-level (which is what the attached
| | > | patch does). Can this be applied
| | >
| | > This would also work, but would it be better to eliminate the
| | > requirement that mark_as_command may only be called at the top level?
| | > Or would the potential for confusion be too great?#
| | >   
| | Your call, though I can see that the above is a nasty syntax error whose
| | reason is not clear. I suppose that a warning might be added for when
| | functions are marked as commands within other functions, and I can turn
| | this warning off in the case of the package manager, so do it however
| | you want.
| | 
| | Note though that the change in the patch I sent for extract_pkgadddel
| | sub-function is necessary as there is a variable name clash in this
| | function that I accidentally introduced..
| 
| For now I think it is best to use evalin.  Will you please check in
| your patch?

Wait, I think the change below may be a better fix.  It simply forces
the PKG_ADD and PKG_DEL files to be sourced in the base workspace.
No change should be needed to the pkg.m file now since executing the
PKG_ADD and PKG_DEL files is handled internally by the addpath and
rmpath functions.

The source_file function in parse.y now has a context arg.  This
functionality is also available in the scripting language, so

  source ("file", "base")

is now the same as

  evalin ('source ("file")', "base")

Do you see any problems with this change?

jwe


src/ChangeLog:

2006-08-29  John W. Eaton  <address@hidden>

        * load-path.cc (execute_pkg_add_or_del):
        Source PKG_ADD or PKG_DEL in base workspace.
        * parse.y (source_file): New optional arg, context.
        * parse.h (source_file): Fix decl.


Index: src/load-path.cc
===================================================================
RCS file: /cvs/octave/src/load-path.cc,v
retrieving revision 1.7
diff -u -u -r1.7 load-path.cc
--- src/load-path.cc    14 Aug 2006 16:42:03 -0000      1.7
+++ src/load-path.cc    29 Aug 2006 16:09:14 -0000
@@ -1211,7 +1211,7 @@
   file_stat fs = file_stat (file);
 
   if (fs.exists ())
-    source_file (file);
+    source_file (file, "base");
 
   unwind_protect::run_frame ("execute_pkg_add_or_del");
 }
Index: src/parse.h
===================================================================
RCS file: /cvs/octave/src/parse.h,v
retrieving revision 1.40
diff -u -u -r1.40 parse.h
--- src/parse.h 16 Aug 2006 06:52:19 -0000      1.40
+++ src/parse.h 29 Aug 2006 16:09:14 -0000
@@ -109,7 +109,8 @@
 load_fcn_from_file (symbol_record *sym_rec, bool exec_script);
 
 extern void
-source_file (const std::string file_name);
+source_file (const std::string& file_name,
+            const std::string& context = std::string ());
 
 extern octave_value_list
 feval (const std::string& name,
Index: src/parse.y
===================================================================
RCS file: /cvs/octave/src/parse.y,v
retrieving revision 1.267
diff -u -u -r1.267 parse.y
--- src/parse.y 16 Aug 2006 06:55:23 -0000      1.267
+++ src/parse.y 29 Aug 2006 16:09:15 -0000
@@ -3527,7 +3527,7 @@
 }
 
 void
-source_file (const std::string file_name)
+source_file (const std::string& file_name, const std::string& context)
 {
   std::string file_full_name = file_ops::tilde_expand (file_name);
 
@@ -3539,11 +3539,26 @@
   curr_fcn_file_name = file_name;
   curr_fcn_file_full_name = file_full_name;
 
-  parse_fcn_file (file_full_name, true, true);
+  if (! context.empty ())
+    {
+      unwind_protect_ptr (curr_sym_tab);
 
-  if (error_state)
-    error ("source: error sourcing file `%s'",
-          file_full_name.c_str ());
+      if (context == "caller")
+       curr_sym_tab = curr_caller_sym_tab;
+      else if (context == "base")
+       curr_sym_tab = top_level_sym_tab;
+      else
+       error ("source: context must be \"caller\" or \"base\"");
+    }      
+
+  if (! error_state)
+    {
+      parse_fcn_file (file_full_name, true, true);
+
+      if (error_state)
+       error ("source: error sourcing file `%s'",
+              file_full_name.c_str ());
+    }
 
   unwind_protect::run_frame ("source_file");
 }
@@ -3629,12 +3644,22 @@
 
   int nargin = args.length ();
 
-  if (nargin == 1)
+  if (nargin == 1 || nargin == 2)
     {
       std::string file_name = args(0).string_value ();
 
       if (! error_state)
-        source_file (file_name);
+       {
+         std::string context;
+
+         if (nargin == 2)
+           context = args(1).string_value ();
+
+         if (! error_state)
+           source_file (file_name, context);
+         else
+           error ("source: expecting context to be character string");
+       }
       else
        error ("source: expecting file name as argument");
     }