LibG lookup logic update to include more clients and more tolerant #9309

QilongTang · 2018-12-12T08:03:15Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

DYN-974
As a Dynamo Client, I want LibG fallback to most compatible major version.

This PR:

Update the ASM lookup logic so DynamoShapeManager will look for ASM NOT ONLY in Revit but also other clients like Civil3D. Currently it is set as a private property in DynamoShapeManager.Utilities, and we can expose this property later and ask clients to overwrite

        private static readonly List<string> ProductsWithASM = new List<string>() { "Revit", "Civil", "FormIt" };

Debug picture sample as below, you can see both Revit and Civil3D with ASM are found:

Update the lookup logic instead of just looking for exact matching version of ASM, look for the best match within same major version of ASM which can be
- perfect match or
- lowest existing one within the same major if fail to find a perfect match. e.g. DynamoShapeManager Looking for latest ASM 225.5.0 delivered with R2020.x but user only have ASM 225.0.0 delivered with R2020 locally, DynamoShapeManager will try to load ASM from there. This would make sure DynamoSandbox is more tolerant finding a compatible and relatively latest version of ASM on user computer to work with.

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.
Running self-CI and all libG preload tests should pass including the new ones added in this PR:
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@aparajit-pratap @ColinDayOrg @mjkkirschner @alfarok

FYIs

@kronz

src/Tools/DynamoShapeManager/Utilities.cs

ColinDayOrg · 2018-12-12T12:50:30Z

I didn't see any reviewers assigned so I took a look. Other than two minor comments, LGTM.

src/DynamoApplications/StartupUtils.cs

src/Tools/DynamoInstallDetective/ProductLookUp.cs

src/Tools/DynamoInstallDetective/Utilities.cs

src/Tools/DynamoShapeManager/Utilities.cs

QilongTang · 2018-12-12T22:29:56Z

Addressed all the comments, installing latest Civil3D to test.

src/Tools/DynamoInstallDetective/ProductLookUp.cs

src/Tools/DynamoShapeManager/Utilities.cs

QilongTang · 2018-12-13T05:33:26Z

@mjkkirschner @aparajit-pratap
Still getting following exception although Sandbox is already getting the correct version:

ASM version from latest Civil3D:

Maybe it is worth to test by copying the 225 final release into the install folder?

QilongTang · 2018-12-13T22:43:43Z

After installing VC++ 2015.3 v14.00 (v140) toolset for desktop, latest testing showing the algorithm works with AutoCAD:

mjkkirschner · 2018-12-14T02:13:30Z

src/Tools/DynamoInstallDetective/Utilities.cs

        /// version info as Tuple of the file found in the installation based 
        /// on file search pattern. The returned list is sorted based on version 
        /// info.</returns>
+        [Obsolete("Function will be deprecated in Dynamo 3.0, please use FindProductInstallations2")]


this message seems a bit odd to me - in Dynamo 3.0 we can just use this method again - now we have to obsolete this method and use the new one.

If no objection, I will change to say the function signature will change in Dynamo 3.0?

src/Tools/DynamoShapeManager/Utilities.cs

mjkkirschner · 2018-12-14T02:19:20Z

@QilongTang this looks solid - a couple thoughts for you, and can you add a test to the preloaderTests file which tries to find a faked folder using the lookup logic - I think some of the other tests in there should be an okay model to follow.

QilongTang · 2018-12-14T15:16:18Z

I found a bug during unit testing in the case that when multiple ADSK products there delivering same version of ASM, I am going to try use Lookup<key,value> instead of Dictionary for version to ASM location mapping

mjkkirschner · 2018-12-14T15:40:45Z

@QilongTang one thing I would like you to think about is making this same behavior work for clients who load ASM directly - like revit- did you get a chance to make this behavior work for those clients that call PreloadASM directly? - I think thats actually more important than making sandbox more robust. ⚠️ like here: https://github.com/DynamoDS/DynamoRevit/blob/master/src/DynamoRevit/DynamoRevit.cs#L485

src/DynamoApplications/StartupUtils.cs

aparajit-pratap · 2018-12-14T16:07:12Z

src/Tools/DynamoInstallDetective/Utilities.cs

+        [Obsolete("Function signature will change in Dynamo 3.0, please refer to FindProductInstallations2")]
        public static IEnumerable FindProductInstallations(string productSearchPattern, string fileSearchPattern)
+        {
+            return FindProductInstallations2(new List<string>() { productSearchPattern }, fileSearchPattern);


Since the new function has a different parameter type than the old one, it can keep the same name, correct? You don't really need to append the 2 to the name?

They have the same number of parameters, but because the types are sufficiently different I think it might be okay to have the same name.

Sometimes unexpected source level breaks occur when adding overloads -
https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net

I don't think we would hit that here since these are not interfaces and the types should not be overlapping.

@mjkkirschner @aparajit-pratap Good point, I will keep the original function and remove the obsolete message. However, I can't keep the same function name because this will introduce ambiguity since the function is currently being called by reflection.

…ducts

QilongTang · 2018-12-14T19:14:12Z

@mjkkirschner @aparajit-pratap All the CI tests pass, I had one error on DateTimeNow which should be a glitch. PTAL.

@mjkkirschner I plan to add the libg folder validation in a different PR

mjkkirschner · 2018-12-14T19:29:30Z

LGTM

alfarok · 2018-12-14T19:34:10Z

test/DynamoCoreTests/libGPreloaderTests.cs

                    new Version(223,0,1),
                    new Version(222,0,0),
                    new Version(221,0,0),
-                    new Version(224,7,0)


@QilongTang I know this is just a test but double checking version 224 or 224.4 is not required?

I can leave it there but as long as a newer version below is there, we will search by the latest so in this test it does not make a difference. Do you suggest another test which contains multiple newer versions within the same major?

@alfarok This case of 224.7.0 falling back to 224.4.0 is equivalent to the test I added below of 225.4.0 falling back to 225.0.0

@QilongTang Not a big deal, I wasn't sure why only 224,7,0 was removed. I also thought maybe it was supposed to by 224,4,0

Great. I know this ASM could be confusing for multiple clients, let me find a good time to knowledge share with you guys.

alfarok · 2018-12-14T19:36:29Z

test/DynamoCoreTests/libGPreloaderTests.cs

+                    new Version(221,0,0),
+                    // Notice the lookup version here is different than the actual found version below
+                    // because there is no local mocked product with that specific ASM version installed
+                    new Version(225,4,0)


should this be 225,0,0?

We specifically want to make it 225,4,0 in this test because this test is to test the fall back case that user does not have an exact matching ASM locally but has a version within same major. In this case, we want to load that version instead of stepping down or fallback to ASM in libG folder

alfarok · 2018-12-14T19:37:22Z

test/DynamoCoreTests/libGPreloaderTests.cs

+
+            };
+
+            var newestASM = new Version(225, 4, 0);


Sorry same question here, does the 4 not represent the minor version?

QilongTang · 2018-12-14T19:42:08Z

test/DynamoCoreTests/libGPreloaderTests.cs

+                versions, ref foundPath, rootFolder, (path) => { return mockedInstalledASMs; });
+
+            // The found version in this case is a fallback of lowest version within same major which should be 225.0.0
+            Assert.AreNotEqual(newestASM, foundVersion);


@alfarok See here, the newest (which is target version) and found are meant to be different.

QilongTang · 2018-12-14T20:01:26Z

Merging.

update libG lookup logic to include more clients and more tolerent

a25aabb

QilongTang added the WIP label Dec 12, 2018

QilongTang changed the title ~~Update libG lookup logic to include more clients and more tolerant~~ LibG lookup logic update to include more clients and more tolerant Dec 12, 2018

QilongTang added 2.x Issues related to 2.x versions of Dynamo. Improvement labels Dec 12, 2018

ColinDayOrg reviewed Dec 12, 2018

View reviewed changes

src/Tools/DynamoShapeManager/Utilities.cs Show resolved Hide resolved

ColinDayOrg reviewed Dec 12, 2018

View reviewed changes

src/Tools/DynamoShapeManager/Utilities.cs Outdated Show resolved Hide resolved