Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for loading ASM from RSA install location #9884

Merged
merged 2 commits into from
Aug 7, 2019
Merged

Add support for loading ASM from RSA install location #9884

merged 2 commits into from
Aug 7, 2019

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Aug 6, 2019 •
edited
Loading

Purpose

JIRA: https://jira.autodesk.com/browse/DYN-2047

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@QilongTang @mjkkirschner

aparajit-pratap
aparajit-pratap commented Aug 6, 2019
View reviewed changes
src/Tools/DynamoShapeManager/Utilities.cs
@@ -14,7 +14,7 @@ public static class Utilities
/// <summary>
/// Key words for Products containing ASM binaries
/// </summary>
private static readonly List<string> ProductsWithASM = new List<string>() { "Revit", "Civil", "FormIt" };
private static readonly List<string> ProductsWithASM = new List<string>() { "Revit", "Civil", "Robot Structural Analysis", "FormIt" };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASM from RSA will be used if there is no Revit or Civil3D installed on the user's machine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot if changing the order here affect the ASM paths found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. See:

Dynamo/src/Tools/DynamoShapeManager/Utilities.cs

Line 154 in 7a392ef

foreach (KeyValuePair<string, Tuple<int, int, int, int>> install in installations)

We first iterate over the different LibG versions and then over the supported products list and whichever matches first, comes out the winner. So it does matter if we play with the order. For example if Revit and RSA are both installed, and there is a match, ASM will be loaded from Revit and if we swap Revit with RSA, ASM will be loaded from RSA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is refreshing to me. Keeping the order of Revit first looks safe.

@aparajit-pratap aparajit-pratap changed the title support for loading ASM from RSA install location Add support for loading ASM from RSA install location Aug 6, 2019
QilongTang
QilongTang reviewed Aug 6, 2019
View reviewed changes
src/Tools/DynamoInstallDetective/ProductLookUp.cs
@@ -159,7 +159,7 @@ static string GetDisplayName(RegistryKey key)
public InstalledProductLookUp(string lookUpName, string fileLookup)
{
this.ProductLookUpName = lookUpName;
this.fileLocator = (path) => Directory.GetFiles(path, fileLookup).FirstOrDefault();
this.fileLocator = (path) => Directory.GetFiles(path, fileLookup, SearchOption.AllDirectories).FirstOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aparajit-pratap Did you notice any noticeable slow down with such searching option with RSA?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice any difference as I don't suspect there is a noticeable slowdown if any after this change. In any case, I think this part of the fix might be unavoidable due to the fact that there is no guarantee that ASM will always be found in the root directory of an installation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aparajit-pratap Thank you, sounds reasonable here.

@QilongTang QilongTang added the LGTM Looks good to me label Aug 6, 2019
QilongTang
QilongTang approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Aug 7, 2019 •
edited
Loading

Thanks @QilongTang for the review. Does this need to be cherry-picked into 2.4?

@aparajit-pratap aparajit-pratap merged commit 7813b83 into DynamoDS:master Aug 7, 2019
@aparajit-pratap aparajit-pratap deleted the dyn-2047 branch August 7, 2019 00:31
aparajit-pratap added a commit to aparajit-pratap/Dynamo that referenced this pull request Aug 7, 2019
@aparajit-pratap
Add support for loading ASM from RSA install location (DynamoDS#9884)
127559c 
* support for loading ASM from RSA install location

* cleanup
aparajit-pratap added a commit to aparajit-pratap/Dynamo that referenced this pull request Aug 7, 2019
@aparajit-pratap
Add support for loading ASM from RSA install location (DynamoDS#9884)
2841b17 
* support for loading ASM from RSA install location

* cleanup
@aparajit-pratap aparajit-pratap mentioned this pull request Aug 7, 2019
Merged
7 tasks
aparajit-pratap added a commit that referenced this pull request Aug 7, 2019
@aparajit-pratap
Add support for loading ASM from RSA install location (#9884) (#9886)
35e1360 
* support for loading ASM from RSA install location

* cleanup
aparajit-pratap added a commit that referenced this pull request Aug 8, 2019
@aparajit-pratap
Revert "Add support for loading ASM from RSA install location (#9884)"
e037111 
This reverts commit 7813b83.
@aparajit-pratap aparajit-pratap mentioned this pull request Aug 8, 2019
Merged
aparajit-pratap added a commit that referenced this pull request Aug 8, 2019
@aparajit-pratap
Revert "Add support for loading ASM from RSA install location (#9884)" (
8577033 
#9890)

This reverts commit 7813b83.
aparajit-pratap added a commit that referenced this pull request Aug 8, 2019
@aparajit-pratap
Revert "Add support for loading ASM from RSA install location (#9884) (…
fa2112d 
…#9886)"

This reverts commit 35e1360.
aparajit-pratap added a commit that referenced this pull request Aug 8, 2019
@aparajit-pratap
Revert "Add support for loading ASM from RSA install location (#9884) (…
78b42ce 
…#9886)" (#9891)

This reverts commit 35e1360.
@aparajit-pratap aparajit-pratap mentioned this pull request Aug 12, 2019
Merged
7 tasks
aparajit-pratap added a commit that referenced this pull request Aug 12, 2019
@aparajit-pratap
Cherry-pick #9899 - Allow ASM to be loaded from non-root product inst…
338b8e4 
…all directories (#9904)

* Add support for loading ASM from RSA install location (#9884)

* support for loading ASM from RSA install location

* cleanup

* resolve cherry-pick conflicts
aparajit-pratap added a commit that referenced this pull request Aug 23, 2019
@aparajit-pratap
Revert ASM Loading changes (#9932)
508896f 
* Add support for loading ASM from RSA install location (#9884)

* support for loading ASM from RSA install location

* cleanup

* correction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QilongTang QilongTang QilongTang approved these changes

Assignees
No one assigned
Labels
LGTM Looks good to me
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aparajit-pratap @QilongTang