fix: resolve IronPython version showing Unknown in output window#3262
fix: resolve IronPython version showing Unknown in output window#3262
Conversation
3d0ffb0 to
a0f3f29
Compare
|
@devloai Go ahead and conduct your review. |
There was a problem hiding this comment.
PR Summary:
Fixes ReadIPYVersion() in EnvDictionarySeeder.cs which was reporting "Unknown" for IronPython version in the output window. The old code searched incorrect paths (bin/IPY342/ instead of bin/{netcore|netfx}/engines/{IPY342|IPY2712PR}/) and only checked IronPython.dll, missing the IPY2712PR variant pyRevitLabs.IronPython.dll.
Review Summary:
The path structure fix is correct and well-supported by Directory.Build.targets — $(PyRevitEnginesDir) resolves to bin/{netcore|netfx}/engines/, confirming the new candidate paths are accurate. Adding both DLL name variants is also the right call.
However, one stated fix didn't land: the PR description claims bug #3 ("executing assembly checked last") was corrected, but selfDir is still appended after all 4 repo candidate dirs in the implementation. This means a different engine's DLL found in the repo layout will still win over the active engine — the exact regression the PR aimed to address. The review checklist also flags silent catch blocks in startup lookup paths as problematic for diagnosability.
Suggestions
dev/pyRevitLoader/pyRevitAssemblyBuilder/SessionManager/EnvDictionarySeeder.cs
Outdated
Show resolved
Hide resolved
dev/pyRevitLoader/pyRevitAssemblyBuilder/SessionManager/EnvDictionarySeeder.cs
Outdated
Show resolved
Hide resolved
|
Review already submitted above (posted simultaneously with your request) — see the review comment for the full breakdown. TL;DR:
|
ReadIPYVersion() had three bugs:
1. Searched wrong paths (bin/IPY342/) instead of the actual layout
(bin/{netcore|netfx}/engines/{IPY342|IPY2712PR}/)
2. Only checked for "IronPython.dll" — missing the IPY2712PR variant
named "pyRevitLabs.IronPython.dll"
3. Checked the executing assembly directory last instead of first,
reporting the wrong engine version when multiple engines are present
Fix: check the active engine directory first (Assembly.GetExecutingAssembly
sits alongside the active IronPython DLL), use correct bin/ layout for
fallback paths, check both DLL name variants, and add Trace.WriteLine
to the catch block for diagnosability.
a0f3f29 to
a07d029
Compare
|
@devloai pushed a new commit, please review according to the above. |
There was a problem hiding this comment.
Re-review after latest commit
All three bugs and the code smell flagged in the previous review are now addressed:
- ✅ Bug #1 fixed: Path structure corrected to
bin/{netcore|netfx}/engines/{IPY342|IPY2712PR}/ - ✅ Bug #2 fixed: Both
IronPython.dllandpyRevitLabs.IronPython.dllare now checked - ✅ Bug #3 fixed:
selfDir(active engine directory) is now added first tocandidateDirs— active engine version wins - ✅ Code smell fixed:
catchblock now logs viaTrace.WriteLineinstead of silently swallowing exceptions
No further issues found. LGTM 🚀
|
Hi @tay0thman I don't like this sort of relative imports that we use now, but I believe that with the architecture we use there is not so much of a choice! Anyways, thanks for patching it out. Letting it through |
|
📦 New work-in-progress (wip) builds are available for 6.3.0.26095+0645-wip |
|
📦 New public release are available for 6.3.0.26095+0830 |
ReadIPYVersion() searched wrong paths (bin/IPY342/) instead of the actual layout (bin/{netcore|netfx}/engines/{IPY342|IPY2712PR}/), and only checked for "IronPython.dll" — missing the IPY2712PR variant named "pyRevitLabs.IronPython.dll".
Fix: use correct directory structure with netcore/netfx and engines segments, and check both DLL name variants in each candidate directory.
ReadIPYVersion() had three bugs:
(bin/{netcore|netfx}/engines/{IPY342|IPY2712PR}/)
named "pyRevitLabs.IronPython.dll"
reporting the wrong engine version when multiple engines are present
Fix: check the active engine directory first (Assembly.GetExecutingAssembly
sits alongside the active IronPython DLL), use correct bin/ layout for
fallback paths, and check both DLL name variants.
"@