-
Notifications
You must be signed in to change notification settings - Fork 636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent crash when starting DynamoSandbox without ASM #9210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dewb thanks for looking into this!
LGTM - potentially we can cherry pick this to 2.0.2 branch as that branch also has these changes around libG loading.
we can also add a test like this to the libGPreloaderTests.cs file
I've verified this would fail before your fix. |
var libGFolderName = ""; | ||
if (version != null) | ||
{ | ||
libGFolderName = string.Format("libg_{0}_{1}_{2}", version.Major, version.Minor, version.Build); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If version is null, then libGFolderName won't be correct (empty). And then preloaderLocation/geometryFactoryPath will not be pointing to any dlls, but they will not be empty either. Worse yet, geometryFactoryPath will not even be a correct path.
I think it would be better that preloaderLocation/geometryFactoryPath be empty so that you can later check if they're not empty.
libGFolderName = string.Format("libg_{0}_{1}_{2}", version.Major, version.Minor, version.Build); | |
if (version == null) | |
{ | |
return; | |
} | |
var libGFolderName = string.Format("libg_{0}_{1}_{2}", version.Major, version.Minor, version.Build); |
Just returning will/should keep preloaderLocation/geometryFactoryPath not set/empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is a good point. I'm not sure what the consequences of leaving these paths empty might be. The intent of this change is just to fix the crash, so preserving the earlier behavior of setting up valid but nonexistent libg_0
path seems the safest route. Additional work in this area would definitely be warranted.
Made a change in response to @hauswij's comments, and added @mjkkirschner's test case. |
@Dewb @mjkkirschner @Racel @aparajit-pratap Not trying to revise what we commit to do here. I am reviewing how this case has been working in history. I do recall it does not crash in Dynamo 1.X when I joined the team. And in my home machine, I could not reproduce the crash after uninstalling Revit. I was able to launch DynamoSandbox in Do you guys recall the behavior in previous versions? |
@QilongTang I do not remember Dynamo starting in the absence of ASM. In any case I tried running Dynamo sandbox 1.3.4 without ASM and it does not (crashes). |
@QilongTang I believe there has never before been a test for this functionality - so it's gone back and forth, I believe I broke it in 2.0.2/master with the work for loading different versions of ASM with the same major version. |
@mjkkirschner That makes sense. I was only curious because to me it works in 2.0.1. @Dewb We will test shortly and let you know |
@mjkkirschner @QilongTang I'm confused. Are you sure you can startup sandbox without ASM in 2.0? |
@aparajit-pratap I am sure for 2.0.1, not sure for 1.3 |
Merging this and ready to test in the coming sprint |
Purpose
Prevent a crash when starting DynamoSandbox on a system where ASM is not (or cannot be) found.
Declarations
Check these if you believe they are true
*.resx
files