From c81ed537c3a94f487e6920e2dfbf837f8a892643 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Sat, 4 Apr 2026 12:38:57 -0400 Subject: [PATCH] Report API plugin load errors to the user --- common/api/api_plugin.cpp | 55 ++++++++--- common/api/api_plugin_manager.cpp | 99 ++++++++++++++++--- common/tool/common_control.cpp | 22 +++++ eeschema/tools/sch_editor_control.cpp | 11 --- eeschema/tools/sch_editor_control.h | 1 - include/api/api_plugin.h | 8 +- include/api/api_plugin_manager.h | 7 +- include/tool/common_control.h | 1 + .../dialogs/panel_pcbnew_action_plugins.cpp | 21 +++- pcbnew/dialogs/panel_pcbnew_action_plugins.h | 3 + 10 files changed, 189 insertions(+), 39 deletions(-) diff --git a/common/api/api_plugin.cpp b/common/api/api_plugin.cpp index f524f08835..48dc581e0d 100644 --- a/common/api/api_plugin.cpp +++ b/common/api/api_plugin.cpp @@ -46,24 +46,31 @@ void LOGGING_ERROR_HANDLER::error( const nlohmann::json::json_pointer& ptr, wxLogTrace( traceApi, wxString::Format( wxS( "JSON error: at %s, value:\n%s\n%s" ), ptr.to_string(), instance.dump(), message ) ); + + wxString location = wxString::FromUTF8( ptr.to_string() ); + + if( location.IsEmpty() ) + location = wxS( "/" ); + + if( !m_errorMessage.IsEmpty() ) + m_errorMessage << '\n'; + + m_errorMessage << wxString::Format( _( "invalid plugin configuration at '%s': %s" ), + location, wxString::FromUTF8( message ) ); } -bool PLUGIN_RUNTIME::FromJson( const nlohmann::json& aJson ) +tl::expected PLUGIN_RUNTIME::FromJson( const nlohmann::json& aJson ) { - // TODO move to tl::expected and give user feedback about parse errors - try { type = magic_enum::enum_cast( aJson.at( "type" ).get(), magic_enum::case_insensitive ) .value_or( PLUGIN_RUNTIME_TYPE::INVALID ); - - } - catch( ... ) + catch( std::exception& e ) { - return false; + return tl::unexpected( wxString::Format( _( "invalid plugin runtime: %s" ), e.what() ) ); } return type != PLUGIN_RUNTIME_TYPE::INVALID; @@ -76,6 +83,7 @@ struct API_PLUGIN_CONFIG const JSON_SCHEMA_VALIDATOR& aValidator ); bool valid; + wxString error_message; wxString identifier; wxString name; wxString description; @@ -93,7 +101,10 @@ API_PLUGIN_CONFIG::API_PLUGIN_CONFIG( API_PLUGIN& aParent, const wxFileName& aCo valid = false; if( !aConfigFile.IsFileReadable() ) + { + error_message = _( "could not read plugin configuration file" ); return; + } wxLogTrace( traceApi, "Plugin: parsing config file" ); @@ -108,9 +119,11 @@ API_PLUGIN_CONFIG::API_PLUGIN_CONFIG( API_PLUGIN& aParent, const wxFileName& aCo /* allow_exceptions = */ true, /* ignore_comments = */ true ); } - catch( ... ) + catch( const std::exception& e ) { wxLogTrace( traceApi, "Plugin: exception during parse" ); + error_message = wxString::Format( _( "plugin configuration file error: %s" ), + wxString::FromUTF8( e.what() ) ); return; } @@ -119,6 +132,8 @@ API_PLUGIN_CONFIG::API_PLUGIN_CONFIG( API_PLUGIN& aParent, const wxFileName& aCo if( !handler.HasError() ) wxLogTrace( traceApi, "Plugin: schema validation successful" ); + else + error_message = handler.ErrorMessage(); // All of these are required; any exceptions here leave us with valid == false try @@ -127,15 +142,21 @@ API_PLUGIN_CONFIG::API_PLUGIN_CONFIG( API_PLUGIN& aParent, const wxFileName& aCo name = js.at( "name" ).get(); description = js.at( "description" ).get(); - if( !runtime.FromJson( js.at( "runtime" ) ) ) + if( !runtime.FromJson( js.at( "runtime" ) ).or_else( + [this]( const wxString& aError ) + { + wxLogTrace( traceApi, "Plugin %s: %s", identifier, aError ); + error_message = aError; + } ).has_value() ) { - wxLogTrace( traceApi, "Plugin: error parsing runtime section" ); return; } } - catch( ... ) + catch( const std::exception& e ) { wxLogTrace( traceApi, "Plugin: exception while parsing required keys" ); + error_message = wxString::Format( _( "missing or invalid required keys: %s" ), + wxString::FromUTF8( e.what() ) ); return; } @@ -143,6 +164,7 @@ API_PLUGIN_CONFIG::API_PLUGIN_CONFIG( API_PLUGIN& aParent, const wxFileName& aCo { wxLogTrace( traceApi, wxString::Format( "Plugin: identifier %s does not meet requirements", identifier ) ); + error_message = wxString::Format( _( "identifier '%s' is invalid" ), identifier ); return; } @@ -166,13 +188,16 @@ API_PLUGIN_CONFIG::API_PLUGIN_CONFIG( API_PLUGIN& aParent, const wxFileName& aCo } } } - catch( ... ) + catch( const std::exception& e ) { wxLogTrace( traceApi, "Plugin: exception while parsing actions" ); + error_message = wxString::Format( _( "actions section is invalid: %s" ), + wxString::FromUTF8( e.what() ) ); return; } valid = true; + error_message = wxEmptyString; } @@ -194,6 +219,12 @@ bool API_PLUGIN::IsOk() const } +const wxString& API_PLUGIN::ErrorMessage() const +{ + return m_config->error_message; +} + + bool API_PLUGIN::IsValidIdentifier( const wxString& aIdentifier ) { // At minimum, we need a reverse-DNS style identifier with two dots and a 2+ character TLD diff --git a/common/api/api_plugin_manager.cpp b/common/api/api_plugin_manager.cpp index e025c0986a..b0c3f184fc 100644 --- a/common/api/api_plugin_manager.cpp +++ b/common/api/api_plugin_manager.cpp @@ -100,6 +100,17 @@ static void reportPluginActionMessage( REPORTER* aReporter, const wxString& aAct } +static void reportPluginLoadMessage( REPORTER* aReporter, const wxString& aPluginName, + const wxString& aMessage ) +{ + if( !aReporter || aMessage.IsEmpty() ) + return; + + aReporter->Report( wxString::Format( _( "Plugin '%s': %s" ), aPluginName, aMessage ), + RPT_SEVERITY_ERROR ); +} + + static void reportPluginActionResult( REPORTER* aReporter, const wxString& aActionName, int aRetVal, const wxString& aError ) { @@ -163,8 +174,11 @@ public: }; -void API_PLUGIN_MANAGER::ReloadPlugins( std::optional aDirectoryToScan ) +void API_PLUGIN_MANAGER::ReloadPlugins( std::optional aDirectoryToScan, + std::shared_ptr aReporter ) { + m_reloadReporter = std::move( aReporter ); + m_plugins.clear(); m_pluginsCache.clear(); m_actionsCache.clear(); @@ -183,17 +197,20 @@ void API_PLUGIN_MANAGER::ReloadPlugins( std::optional aDirectoryToScan if( plugin->IsOk() ) { - if( m_pluginsCache.count( plugin->Identifier() ) ) + const wxString& id = plugin->Identifier(); + + if( m_pluginsCache.contains( id ) ) { - wxLogTrace( traceApi, - wxString::Format( "Manager: identifier %s already present!", - plugin->Identifier() ) ); + wxLogTrace( traceApi, wxString::Format( "Manager: identifier %s already present, reloading", id ) ); + + for( const PLUGIN_ACTION& action : m_pluginsCache[id]->Actions() ) + m_actionsCache[action.identifier] = &action; + + m_pluginsCache.erase( id ); return; } - else - { - m_pluginsCache[plugin->Identifier()] = plugin.get(); - } + + m_pluginsCache[id] = plugin.get(); for( const PLUGIN_ACTION& action : plugin->Actions() ) m_actionsCache[action.identifier] = &action; @@ -203,6 +220,9 @@ void API_PLUGIN_MANAGER::ReloadPlugins( std::optional aDirectoryToScan else { wxLogTrace( traceApi, "Manager: loading failed" ); + + reportPluginLoadMessage( m_reloadReporter.get(), aFile.GetFullPath(), + plugin->ErrorMessage() ); } } ); @@ -253,6 +273,9 @@ void API_PLUGIN_MANAGER::ReloadPlugins( std::optional aDirectoryToScan processPluginDependencies(); + if( !Busy() ) + m_reloadReporter.reset(); + wxCommandEvent* evt = new wxCommandEvent( EDA_EVT_PLUGIN_AVAILABILITY_CHANGED, wxID_ANY ); m_parent->QueueEvent( evt ); } @@ -343,7 +366,7 @@ int API_PLUGIN_MANAGER::doInvokeAction( const wxString& aIdentifier, std::vector wxLogTrace( traceApi, wxString::Format( "Manager: Python interpreter for %s not found", plugin.Identifier() ) ); reportPluginActionMessage( aReporter.get(), action->name, - _( "Python environment does not exist" ) ); + _( "missing plugin environment" ) ); return -1; } @@ -709,7 +732,7 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) }; manager.Execute( args, - [this]( int aRetVal, const wxString& aOutput, const wxString& aError ) + [this, job]( int aRetVal, const wxString& aOutput, const wxString& aError ) { wxLogTrace( traceApi, wxString::Format( "Manager: created venv (python returned %d)", aRetVal ) ); @@ -717,6 +740,19 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) if( !aError.IsEmpty() ) wxLogTrace( traceApi, wxString::Format( "Manager: venv err: %s", aError ) ); + if( aRetVal != 0 ) + { + wxString error = aError; + error.Trim().Trim( false ); + + if( error.IsEmpty() ) + error = wxString::Format( _( "error code %d" ), aRetVal ); + + error = wxString::Format( _( "could not create plugin environment: %s" ), error ); + + reportPluginLoadMessage( m_reloadReporter.get(), job.identifier, error ); + } + wxCommandEvent* evt = new wxCommandEvent( EDA_EVT_PLUGIN_MANAGER_JOB_FINISHED, wxID_ANY ); QueueEvent( evt ); @@ -738,6 +774,9 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) { wxLogTrace( traceApi, wxString::Format( "Manager: error: python not found at %s", job.env_path ) ); + + reportPluginLoadMessage( m_reloadReporter.get(), job.identifier, + _( "missing plugin environment" ) ); } else { @@ -772,7 +811,7 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) }; manager.Execute( args, - [this]( int aRetVal, const wxString& aOutput, const wxString& aError ) + [this, job]( int aRetVal, const wxString& aOutput, const wxString& aError ) { wxLogTrace( traceApi, wxString::Format( "Manager: upgrade pip returned %d", aRetVal ) ); @@ -783,6 +822,19 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) wxString::Format( "Manager: upgrade pip stderr: %s", aError ) ); } + if( aRetVal != 0 ) + { + wxString error = aError; + error.Trim().Trim( false ); + + if( error.IsEmpty() ) + error = wxString::Format( _( "error code %d" ), aRetVal ); + + error = wxString::Format( _( "could not create plugin environment: %s" ), error ); + + reportPluginLoadMessage( m_reloadReporter.get(), job.identifier, error ); + } + wxCommandEvent* evt = new wxCommandEvent( EDA_EVT_PLUGIN_MANAGER_JOB_FINISHED, wxID_ANY ); QueueEvent( evt ); @@ -806,12 +858,18 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) { wxLogTrace( traceApi, wxString::Format( "Manager: error: python not found at %s", job.env_path ) ); + + reportPluginLoadMessage( m_reloadReporter.get(), job.identifier, + _( "missing plugin environment" ) ); } else if( !reqs.IsFileReadable() ) { wxLogTrace( traceApi, wxString::Format( "Manager: error: requirements.txt not found at %s", job.plugin_path ) ); + + reportPluginLoadMessage( m_reloadReporter.get(), job.identifier, + _( "requirements.txt could not be read" ) ); } else { @@ -854,6 +912,19 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) if( !aError.IsEmpty() ) wxLogTrace( traceApi, wxString::Format( "Manager: pip stderr: %s", aError ) ); + if( aRetVal != 0 ) + { + wxString error = aError; + error.Trim().Trim( false ); + + if( error.IsEmpty() ) + error = wxString::Format( _( "error code %d" ), aRetVal ); + + error = wxString::Format( _( "could not create plugin environment: %s" ), error ); + + reportPluginLoadMessage( m_reloadReporter.get(), job.identifier, error ); + } + if( aRetVal == 0 ) { wxLogTrace( traceApi, wxString::Format( "Manager: marking %s as ready", @@ -879,6 +950,10 @@ void API_PLUGIN_MANAGER::processNextJob( wxCommandEvent& aEvent ) } m_jobs.pop_front(); + + if( !Busy() ) + m_reloadReporter.reset(); + wxLogTrace( traceApi, wxString::Format( "Manager: finished job; %zu left in queue", m_jobs.size() ) ); } diff --git a/common/tool/common_control.cpp b/common/tool/common_control.cpp index e343c7afdc..5cdd4cdbf0 100644 --- a/common/tool/common_control.cpp +++ b/common/tool/common_control.cpp @@ -37,6 +37,8 @@ #include #include #include + +#include #include #include #include @@ -46,6 +48,8 @@ #include #include #include +#include +#include #define URL_GET_INVOLVED wxS( "https://go.kicad.org/contribute/" ) #define URL_DONATE wxS( "https://go.kicad.org/app-donate" ) @@ -425,6 +429,23 @@ int COMMON_CONTROL::ReportBug( const TOOL_EVENT& aEvent ) } +int COMMON_CONTROL::ReloadPlugins( const TOOL_EVENT& aEvent ) +{ +#ifdef KICAD_IPC_API + if( Pgm().GetCommonSettings()->m_Api.enable_server ) + { + std::shared_ptr reporter; + + if( KISTATUSBAR* statusBar = dynamic_cast( m_frame->GetStatusBar() ) ) + reporter = std::make_shared( statusBar, wxS( "plugin" ) ); + + Pgm().GetPluginManager().ReloadPlugins( std::nullopt, reporter ); + } +#endif + return 0; +} + + void COMMON_CONTROL::setTransitions() { Go( &COMMON_CONTROL::Quit, ACTIONS::quit.MakeEvent() ); @@ -448,6 +469,7 @@ void COMMON_CONTROL::setTransitions() Go( &COMMON_CONTROL::Donate, ACTIONS::donate.MakeEvent() ); Go( &COMMON_CONTROL::ReportBug, ACTIONS::reportBug.MakeEvent() ); Go( &COMMON_CONTROL::About, ACTIONS::about.MakeEvent() ); + Go( &COMMON_CONTROL::ReloadPlugins, ACTIONS::pluginsReload.MakeEvent() ); } diff --git a/eeschema/tools/sch_editor_control.cpp b/eeschema/tools/sch_editor_control.cpp index bbe433f9a4..deb7e44c2e 100644 --- a/eeschema/tools/sch_editor_control.cpp +++ b/eeschema/tools/sch_editor_control.cpp @@ -3316,15 +3316,6 @@ int SCH_EDITOR_CONTROL::ToggleAnnotateAuto( const TOOL_EVENT& aEvent ) } -int SCH_EDITOR_CONTROL::ReloadPlugins( const TOOL_EVENT& aEvent ) -{ -#ifdef KICAD_IPC_API - if( Pgm().GetCommonSettings()->m_Api.enable_server ) - Pgm().GetPluginManager().ReloadPlugins(); -#endif - return 0; -} - int SCH_EDITOR_CONTROL::OnAngleSnapModeChanged( const TOOL_EVENT& aEvent ) { // Update the left toolbar Line modes group icon to match current mode @@ -3573,8 +3564,6 @@ void SCH_EDITOR_CONTROL::setTransitions() Go( &SCH_EDITOR_CONTROL::OnAngleSnapModeChanged, SCH_ACTIONS::angleSnapModeChanged.MakeEvent() ); Go( &SCH_EDITOR_CONTROL::ToggleAnnotateAuto, SCH_ACTIONS::toggleAnnotateAuto.MakeEvent() ); - Go( &SCH_EDITOR_CONTROL::ReloadPlugins, ACTIONS::pluginsReload.MakeEvent() ); - Go( &SCH_EDITOR_CONTROL::ExportSymbolsToLibrary, SCH_ACTIONS::exportSymbolsToLibrary.MakeEvent() ); Go( &SCH_EDITOR_CONTROL::PlaceLinkedDesignBlock, SCH_ACTIONS::placeLinkedDesignBlock.MakeEvent() ); diff --git a/eeschema/tools/sch_editor_control.h b/eeschema/tools/sch_editor_control.h index 1e4c3fce47..e3cfe44f6d 100644 --- a/eeschema/tools/sch_editor_control.h +++ b/eeschema/tools/sch_editor_control.h @@ -149,7 +149,6 @@ public: int NextLineMode( const TOOL_EVENT& aEvent ); int OnAngleSnapModeChanged( const TOOL_EVENT& aEvent ); int ToggleAnnotateAuto( const TOOL_EVENT& aEvent ); - int ReloadPlugins( const TOOL_EVENT& aEvent ); int GridFeedback( const TOOL_EVENT& aEvent ); diff --git a/include/api/api_plugin.h b/include/api/api_plugin.h index 2a219c78ab..edf500ab00 100644 --- a/include/api/api_plugin.h +++ b/include/api/api_plugin.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -56,7 +57,7 @@ enum class PLUGIN_RUNTIME_TYPE struct PLUGIN_RUNTIME { - bool FromJson( const nlohmann::json& aJson ); + tl::expected FromJson( const nlohmann::json& aJson ); PLUGIN_RUNTIME_TYPE type; wxString min_version; @@ -100,6 +101,8 @@ public: bool IsOk() const; + const wxString& ErrorMessage() const; + static bool IsValidIdentifier( const wxString& aIdentifier ); const wxString& Identifier() const; @@ -142,11 +145,14 @@ public: bool HasError() const { return m_hasError; } + const wxString& ErrorMessage() const { return m_errorMessage; } + void error( const nlohmann::json::json_pointer& ptr, const nlohmann::json& instance, const std::string& message ) override; private: bool m_hasError; + wxString m_errorMessage; }; #endif //KICAD_API_PLUGIN_H diff --git a/include/api/api_plugin_manager.h b/include/api/api_plugin_manager.h index 6193224bd4..4e88eae535 100644 --- a/include/api/api_plugin_manager.h +++ b/include/api/api_plugin_manager.h @@ -51,7 +51,8 @@ public: * @param aDirectoryToScan can be provided to scan an arbitrary directory instead of the * stock paths; provided for QA testing. */ - void ReloadPlugins( std::optional aDirectoryToScan = std::nullopt ); + void ReloadPlugins( std::optional aDirectoryToScan = std::nullopt, + std::shared_ptr aReporter = nullptr ); void RecreatePluginEnvironment( const wxString& aIdentifier ); @@ -82,6 +83,8 @@ public: std::map& MenuBindings() { return m_menuBindings; } + std::shared_ptr GetReporter() { return m_reloadReporter; } + private: void processPluginDependencies(); @@ -132,6 +135,8 @@ private: std::unique_ptr m_schema_validator; + std::shared_ptr m_reloadReporter; + [[maybe_unused]] long m_lastPid; [[maybe_unused]] wxTimer* m_raiseTimer; }; diff --git a/include/tool/common_control.h b/include/tool/common_control.h index 38646a2821..72f1da0d20 100644 --- a/include/tool/common_control.h +++ b/include/tool/common_control.h @@ -60,6 +60,7 @@ public: int GetInvolved( const TOOL_EVENT& aEvent ); int Donate( const TOOL_EVENT& aEvent ); int ReportBug( const TOOL_EVENT& aEvent ); + int ReloadPlugins( const TOOL_EVENT& aEvent ); ///< Sets up handlers for various events. void setTransitions() override; diff --git a/pcbnew/dialogs/panel_pcbnew_action_plugins.cpp b/pcbnew/dialogs/panel_pcbnew_action_plugins.cpp index e9b74e1194..46ea8b45bf 100644 --- a/pcbnew/dialogs/panel_pcbnew_action_plugins.cpp +++ b/pcbnew/dialogs/panel_pcbnew_action_plugins.cpp @@ -29,13 +29,17 @@ #include #include #include +#include #include #include +#include #include +#include #include #include #include #include +#include #include @@ -123,6 +127,9 @@ PANEL_PCBNEW_ACTION_PLUGINS::PANEL_PCBNEW_ACTION_PLUGINS( wxWindow* aParent ) : m_reloadButton->SetBitmap( KiBitmapBundle( BITMAPS::small_refresh ) ); m_showErrorsButton->SetBitmap( KiBitmapBundle( BITMAPS::small_warning ) ); + m_errorDialog = new DIALOG_HTML_REPORTER( aParent ); + m_allowErrorDialog = false; + wxTheApp->Bind( EDA_EVT_PLUGIN_AVAILABILITY_CHANGED, &PANEL_PCBNEW_ACTION_PLUGINS::onPluginAvailabilityChanged, this ); } @@ -130,6 +137,7 @@ PANEL_PCBNEW_ACTION_PLUGINS::PANEL_PCBNEW_ACTION_PLUGINS( wxWindow* aParent ) : PANEL_PCBNEW_ACTION_PLUGINS::~PANEL_PCBNEW_ACTION_PLUGINS() { + delete m_errorDialog; wxTheApp->Unbind( EDA_EVT_PLUGIN_AVAILABILITY_CHANGED, &PANEL_PCBNEW_ACTION_PLUGINS::onPluginAvailabilityChanged, this ); m_grid->PopEventHandler( true ); @@ -140,6 +148,14 @@ void PANEL_PCBNEW_ACTION_PLUGINS::onPluginAvailabilityChanged( wxCommandEvent& a { m_grid->Enable(); TransferDataToWindow(); + + if( m_allowErrorDialog && m_errorDialog->m_Reporter->HasMessage() ) + { + m_errorDialog->m_Reporter->Flush(); + m_allowErrorDialog = false; + m_errorDialog->ShowModal(); + } + aEvt.Skip(); } @@ -195,7 +211,10 @@ void PANEL_PCBNEW_ACTION_PLUGINS::SwapRows( int aRowA, int aRowB ) void PANEL_PCBNEW_ACTION_PLUGINS::OnReloadButtonClick( wxCommandEvent& event ) { API_PLUGIN_MANAGER& mgr = Pgm().GetPluginManager(); - mgr.ReloadPlugins(); + m_errorDialog->m_Reporter->Clear(); + auto reporter = std::make_shared( m_errorDialog->m_Reporter ); + m_allowErrorDialog = true; + mgr.ReloadPlugins( std::nullopt, reporter ); m_grid->Disable(); } diff --git a/pcbnew/dialogs/panel_pcbnew_action_plugins.h b/pcbnew/dialogs/panel_pcbnew_action_plugins.h index cc179dc284..4448a4067f 100644 --- a/pcbnew/dialogs/panel_pcbnew_action_plugins.h +++ b/pcbnew/dialogs/panel_pcbnew_action_plugins.h @@ -21,6 +21,7 @@ #include "panel_pcbnew_action_plugins_base.h" class PLUGINS_GRID_TRICKS; +class DIALOG_HTML_REPORTER; class PANEL_PCBNEW_ACTION_PLUGINS : public PANEL_PCBNEW_ACTION_PLUGINS_BASE @@ -76,6 +77,8 @@ private: }; wxBitmapBundle m_genericIcon; + DIALOG_HTML_REPORTER* m_errorDialog; + bool m_allowErrorDialog; void SwapRows( int aRowA, int aRowB ); void SelectRow( int aRow );