Forum OpenACS Improvement Proposals (TIPs): Re: Display errors that occurred while sourcing a file in APM package-reload page.

That patch has some errors. One is due to hand-quoting the HTML embedded in the patch (see this bug):
+  <% template::head::add_javascript -src "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js" %<
…should be:
+  <% template::head::add_javascript -src "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js" %>

Not sure if any more of those remain, so caveat emptor.

Also, it occurs to me that, as was expected in the 1st patch in the thread, apm_load_any_changed_libraries will load new code without any attention to package boundaries, so set errors($r_file) $errorInfo needs to be lappend errors($package_key) $r_file $errorInfo with suitable code for getting the package_key from the full file name. This means a number of changes to the patch chunk for acs-admin/www/apm/version-reload.tcl as well.

The patch (fixes in green)



Index: acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
===================================================================
--- acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
+++ acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
@@ -377,25 +379,33 @@
     return [expr ![file exists "${package_path}/sql"] || [file exists "${package_path}/sql/[db_type]"]]
 }
 
-ad_proc -private apm_source { __file } {
+ad_proc -private apm_source { __file {error_set_name ""}} {
     Sources $__file in a clean environment, returning 1 if successful or 0 if not.
     Records that the file has been sourced and stores its mtime in the nsv array
     apm_library_mtime
 } {
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     if { ![file exists $__file] } {
		ns_log "Error" "Unable to source $__file: file does not exist."
 	return 0
     }
 
+    set r_file [ad_make_relative_path $__file]
+
     # Actually do the source.
     if { [catch { source $__file }] } {
 	global errorInfo
		ns_log "Error" "Error sourcing $__file:\n$errorInfo"
+	set package_key ""
+	regexp {/packages/([^/]+)/} $__file -> package_key
+	lappend errors($package_key) $r_file $errorInfo
 	return 0
     }
 
-    nsv_set apm_library_mtime [ad_make_relative_path $__file] [file mtime $__file]    
+    nsv_set apm_library_mtime $r_file [file mtime $__file]

     return 1
 }



Index: acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
===================================================================
--- acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
+++ acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
@@ -217,14 +217,24 @@
 #
 ################################################
 
-ad_proc -public db_qd_load_query_file {file_path} {
+ad_proc -public db_qd_load_query_file {file_path {error_set_name ""}} {
     A procedure that is called from the outside world (APM) 
     to load a particular file
 } { 
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     if { [catch {db_qd_internal_load_cache $file_path} errmsg] } {
         global errorInfo
         ns_log Error "Error parsing queryfile $file_path:\n\n$errmsg\n\n$errorInfo"
+	set r_file [ad_make_relative_path $file_path]
+	set package_key ""
+	regexp {/packages/([^/]+)/} $file_path -> package_key
+	lappend errors($package_key) $r_file $errorInfo
     }
 }



Index: acs-tcl/tcl/apm-procs.tcl
===================================================================
--- acs-tcl/tcl/apm-procs.tcl
+++ acs-tcl/tcl/apm-procs.tcl
@@ -739,7 +739,7 @@
     " -default "all" -bind [list db_type $db_type]]]
 }
 
-ad_proc -public apm_load_any_changed_libraries {} {
+ad_proc -public apm_load_any_changed_libraries {{error_set_name {}}} {
     
     In the running interpreter, reloads files marked for reload by
     apm_mark_version_for_reload. If any watches are set, examines watched
@@ -748,6 +748,10 @@
     before any filters or registered procedures are applied).
 
 } {
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     # Determine the current reload level in this interpreter by calling
     # apm_reload_level_in_this_interpreter. If this fails, we define the reload level to be
     # zero.
@@ -807,12 +811,12 @@
                         # Make sure this is not a -init.tcl file as those should only be sourced on server startup
                         if { ![regexp {\-init\.tcl$} $file_path] } {
                             ns_log Notice "apm_load_any_changed_libraries: Reloading $file..."
-                            apm_source $file_path
+                            apm_source $file_path errors
                         }
                     }
                     .xql { 
                         ns_log Notice "apm_load_any_changed_libraries: Reloading $file..."
-                        db_qd_load_query_file $file_path
+                        db_qd_load_query_file $file_path errors
                     }
                     default {
                         ns_log Notice "apm_load_any_changed_libraries: File $file_path has unknown extension. Not reloading."



Index: acs-admin/www/apm/version-reload.tcl
===================================================================
--- acs-admin/www/apm/version-reload.tcl
+++ acs-admin/www/apm/version-reload.tcl
@@ -23,7 +23,13 @@
 if { [llength $files] == 0 } {
     append body "There are no changed files to reload in this package.<p>"
 } else {
-    append body "Marked the following file[ad_decode [llength $files] 1 "" "s"] for reloading:<ul>\n"
+    append body "Marked the following file[ad_decode [llength $files] 1 "" "s"] for reloading:<ul id='files'>\n"
+
+    # Source all of the marked files using the current interpreter, accumulating
+    # errors into apm_package_load_errors
+    array set errors [list]
+    catch { apm_load_any_changed_libraries errors}
+
+    if {[info exists errors($package_key)]} {
+ 	array set package_errors $errors($package_key)
+    } else {
+ 	array set package_errors [list]
+    }
+
     foreach file $files {
 	append body "<li>$file"
 	if { [nsv_exists apm_reload_watch $file] } {
@@ -38,9 +44,29 @@
 	    append body " (<a href=\"file-watch?[export_vars { version_id { paths $local_path } }]\">watch this file</a>)"
             lappend files_to_watch $local_path
 	}
-	append body "\n"
+
+	if {[info exists package_errors($file)]} {
+	    append body "<dl class='error'><dt title='click to see details'>ERROR! (click for details)</dt><dd><code><pre>[ad_quotehtml $package_errors($file)]</pre></code></dd></dl>"
+	}
+	append body "</li>\n"
     }
     append body "</ul>\n"
+
+    set n_errors [array size package_errors]
+    if {$n_errors > 0} {
+	if {$n_errors != 1} {
+	    set exist_n_error_files "were $n_errors files"
+	} else {
+	    set exist_n_error_files "was $n_errors file"
+	}
+	append body "
+		<p><strong style='color:red;font-size:112.5%;'>There
+		$exist_n_error_files with errors that prevented complete
+		reloading</strong>.  Fix the problem, then reload the
+		package again to finish the reload.
+		</p>
+	"
+    }
 }



Index: acs-admin/www/apm/version-reload.adp
===================================================================
--- acs-admin/www/apm/version-reload.adp
+++ acs-admin/www/apm/version-reload.adp
@@ -2,4 +2,19 @@
 <property name="title">@page_title;noquote@</property>
 <property name="context">@context;noquote@</property>
 
+<style type="text/css">
+  dl.error, dl.error dt { display:inline; color:red; }
+  dl.error dt { font-weight:bold; }
+  dl.error    { margin-left:1em; cursor:pointer; }
+  dl.error dd { display:none; }
+</style>
 @body;noquote@
+<script type="text/javascript">
+  <% template::head::add_javascript -src "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js" %>
+  $('#files dd').click(function () {
+	$(this).slideToggle();
+  });
+  $('#files dt').click(function () {
+	$(this.nextElementSibling).slideToggle();
+  });
+</script>