New C# Loader. Bugfixes#3008
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements bugfixes for the new C# loader in pyRevit, addressing multiple issues related to extension discovery, engine type configuration, and split button functionality. The changes improve cache management, add support for CPython engine type specification, and enhance the robustness of UI component building.
Changes:
- Added engine type configuration support to allow specifying CPython or IronPython explicitly
- Implemented cache clearing mechanism to ensure newly installed extensions are discovered on reload
- Enhanced split button functionality with proper synchronization, null checks, and error handling
Reviewed changes
Copilot reviewed 11 out of 37 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| dev/pyRevitLoader/pyRevitExtensionParser/PyRevitConfig.cs | Added Type property to EngineConfig for explicit engine selection |
| dev/pyRevitLoader/pyRevitExtensionParser/ExtensionParser.cs | Added ClearAllCaches method and third-party extensions directory discovery |
| dev/pyRevitLoader/pyRevitExtensionParser/BundleParser.cs | Added parsing for engine type in bundle configuration |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/SessionManagerService.cs | Integrated cache clearing into session load process |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/Buttons/SplitButtonBuilder.cs | Major enhancement with null checks, error handling, and SmartButton support |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/UIManager/Builders/StackBuilder.cs | Added split button support in stacked layouts |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/SessionManager/ServiceFactory.cs | Updated dependency injection for SplitButtonBuilder |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/SessionManager/IExtensionManagerService.cs | Added ClearParserCaches interface method |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/SessionManager/ExtensionManagerService.cs | Implemented ClearParserCaches method |
| dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs | Changed to root-based path resolution and added context escaping |
| dev/pyRevitLabs.PyRevit.Runtime/scriptruntime.cs | Added engine type detection from JSON configuration |
| bin/netfx/engines/IPY342/pyRevitExtensionParser.dll | Updated compiled binary |
dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs
Show resolved
Hide resolved
dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs
Outdated
Show resolved
Hide resolved
dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
PR Summary:
This PR addresses critical bugfixes for the new C# loader, resolving 4 reported issues:
- Enables extension discovery after install/enable without restart by clearing parser caches
- Adds support for third-party extensions in
%APPDATA%\pyRevit\Extensions - Fixes split/splitpush buttons not counting in stacks and improves their initialization
- Resolves CPython module import issues by properly constructing
pyrevitlibandsite-packagespaths - Adds engine type configuration support for explicit CPython/IronPython selection
Review Summary:
Reviewed C# loader bugfixes addressing extension loading, split button support, and CPython path handling. The PR successfully fixes the reported issues, but introduces 2 high-severity bugs around SmartButton initialization in split buttons that need to be addressed before merging. Additionally identified code quality improvements around exception handling and path resolution robustness.
Key findings:
- ✅ Core bugfixes properly address reported issues #2996-#2999
- ❌ SmartButton support incomplete - missing initialization logic and DI wiring
⚠️ Empty catch block in engine config parsing violates guidelines⚠️ Hardcoded path traversal may fail in non-standard deployments
Knowledge utilized: pyRevit code review validation checklist (exception handling, dependency injection patterns), C# code standards, and build system architecture.
Follow-up suggestions:
@devloai fix comment #1 and #5- Critical SmartButton initialization bugs that must be fixed together@devloai fix all identified issues- Address all 5 review comments
| // Track first button to set as current | ||
| firstButton ??= subBtn; | ||
| childCount++; | ||
| Logger.Debug($"Added child button '{sub.DisplayName}' to split button '{component.DisplayName}' (child #{childCount})."); |
There was a problem hiding this comment.
Missing SmartButton initialization logic
While SmartButton is now supported in split buttons (line 133), the SmartButton initialization logic is missing. In PulldownButtonBuilder.cs, SmartButtons have special handling that calls _smartButtonScriptInitializer.ExecuteSelfInit() (lines 143-151 in PulldownButtonBuilder.cs).
The same logic should be applied here for SmartButtons in split buttons:
else if (sub.Type == CommandComponentType.SmartButton)
{
try
{
var pushButtonData = CreatePushButtonData(sub, assemblyInfo!);
var subBtn = splitBtn.AddPushButton(pushButtonData);
if (subBtn != null)
{
ButtonPostProcessor.Process(subBtn, sub, component);
// Execute __selfinit__ for SmartButton in split button
if (_smartButtonScriptInitializer != null)
{
var shouldActivate = _smartButtonScriptInitializer.ExecuteSelfInit(sub, subBtn);
if (!shouldActivate)
{
subBtn.Enabled = false;
Logger.Debug($"SmartButton '{sub.DisplayName}' in split button deactivated by __selfinit__.");
}
}
firstButton ??= subBtn;
childCount++;
Logger.Debug($"Added SmartButton '{sub.DisplayName}' to split button '{component.DisplayName}' (child #{childCount}).");
}
// ... rest of null handling
}
catch (Exception ex)
{
Logger.Error($"Failed to add SmartButton '{sub.DisplayName}' to split button '{component.DisplayName}'. Exception: {ex.Message}");
}
}
else if (sub.Type == CommandComponentType.PushButton ||
sub.Type == CommandComponentType.UrlButton ||
sub.Type == CommandComponentType.InvokeButton ||
sub.Type == CommandComponentType.ContentButton)
{
// existing code for other button types
}Without this, SmartButtons in split buttons won't have their __selfinit__ scripts executed, which could lead to incorrect button state or missing custom initialization.
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
dev/pyRevitLoader/pyRevitAssemblyBuilder/AssemblyMaker/CommandTypeGenerator.cs
Show resolved
Hide resolved
dev/pyRevitLoader/pyRevitAssemblyBuilder/SessionManager/ServiceFactory.cs
Show resolved
Hide resolved
Testing Cpython command from devtools extension
3.12.3 (tags/v3.12.3:f6650f9, Apr 9 2024, 14:05:25) [MSC v.1938 64 bit (AMD64)] sys.path:C:\Users\Local Admin\Documents\GitHub\pyRevit\bin\cengines\CPY3123\python312.zip This file (file):file = C:\Users\Local Admin\Documents\GitHub\pyRevit\extensions\pyRevitDevTools.extension\pyRevitDev.tab\Debug.panel\Engine Tests.pulldown\Test CPython Command.pushbutton\script.py This scope (name):name = main UIApplication: (revit)revit = Autodesk.Revit.UI.UIApplication pyRevit globals:execid = 3WoyXuabrEmQJEin1B6OTg== timestamp = 2026-01-13T20:15:01.7964Z cachedengine = False cachedengineid = e5fac8e6-1514-4489-9e02-2d70e9632d1f:CPython:pyRevitDevTools scriptruntime = PyRevitLabs.PyRevit.Runtime.ScriptRuntime commanddata = Autodesk.Revit.UI.ExternalCommandData elements = Autodesk.Revit.DB.ElementSet commandpath = C:\Users\Local Admin\Documents\GitHub\pyRevit\extensions\pyRevitDevTools.extension\pyRevitDev.tab\Debug.panel\Engine Tests.pulldown\Test CPython Command.pushbutton configcommandpath = C:\Users\Local Admin\Documents\GitHub\pyRevit\extensions\pyRevitDevTools.extension\pyRevitDev.tab\Debug.panel\Engine Tests.pulldown\Test CPython Command.pushbutton commandname = Test CPython Command commandbundle = Test CPython Command.pushbutton commandextension = pyRevitDevTools commanduniqueid = pyrevitdevtools_pyrevitdev_debug_enginetests_testcpythoncommand forceddebugmode = False shiftclick = False result = System.Collections.Generic.Dictionary`2[System.String,System.String] eventsender = None eventargs = None Module Tests:pythonpath='C:\Users\Local Admin\AppData\Roaming\Python\Python312\site-packages' tkinter load error: No module named 'tkinter' numpy array:array([[ 0, 1, 2, 3, 4], pandas DataFrame:key 1 key 2 key 3 0 Revit Document Tests:list of DB.Walls:Autodesk.Revit.DB.Wall id:2838 Autodesk.Revit.DB.Wall id:2866 Autodesk.Revit.DB.Wall id:2921 Autodesk.Revit.DB.Wall id:2949 Autodesk.Revit.DB.Wall id:3038 Кириллица (/ sɪˈrɪlɪk /) - это система письма pyrevit.revit: |
|
Using WIP installer for #3011: I still have error "No module named pyrevit": error
IronPython Traceback: Script Executor Traceback: Disabling the new loader allowed the script to work again. |
https://github.com/pyrevitlabs/pyRevit/actions/runs/20900562513 Right version wrong engine: 2712 is the iron python engine @frank-e-loftus also provide the script you are testing with, please |
Error triggered at line 8 above: ImportError: No module named pyrevit PR #3008 says it will fix this. It just hasn't been merged yet, right? So I think I just have to wait until #3008 gets merged, then I can test again. My bad, it's probably a non-issue. |
You are right @frank-e-loftus this is not testable through wip installer. You need to clone the repo, checkout the PR, create the pyrevit clone and attach it. |
|
|
@romangolev I could not test the splitpushbutton thing, can you? Then let that sit another week, and then Release, if everyone is happy with it |
|
Hi @jmcouffin. Tested the splitbutton and it seemed to worked fine on my side. Dived deep with reloading processed and recreated the functionality of removing the disabled or deleted extension #2996 , we should be alright here |
|
@jmcouffin I took a look at #3010 and came up with the fix for it, so it should be as well covered with this PR |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26022+2138-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26023+2136-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26023+2141-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26025+1329-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26025+1418-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26030+2037-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26030+2039-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26030+2101-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26030+2136-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26030+2147-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26030+2212-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1043-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1111-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1304-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1323-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1433-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1538-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1543-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1553-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1612-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1624-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1738-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1743-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1829-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.1.26032+1937-wip |
|
📦 New work-in-progress (wip) builds are available for 6.0.0.26032+1956-wip |
|
📦 New work-in-progress (wip) builds are available for 6.0.0.26032+2005-wip |
|
📦 New work-in-progress (wip) builds are available for 6.0.0.26032+2008-wip |
|
📦 New public release are available for 6.0.0.26032+2040 |
|
📦 New public release are available for 6.0.0.26032+2040 |
Related Issues: