Forum OpenACS Improvement Proposals (TIPs): Re: Display errors that occurred while sourcing a file in APM package-reload page.
9:
Re: Display errors that occurred while sourcing a file in APM package-reload page.
(response to 8)
Posted by
Andrew Helsley
on 07/31/14 01:53 AM
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" %<
+ <% 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>
