update for the Dynamo 3.0 release #50

pinzart90 · 2023-12-11T15:04:23Z

Changed how tests are run:
The resolver in Dynamo test infrastructure is broken (missing resolver paths for the main dynamo assemblies and for resources)
Added a custom resolver in the test.

.github/workflows/build_dynamo_samples_net8.0.yml

mjkkirschner · 2023-12-12T20:54:35Z

src/SampleLibraryTests/Setup.cs

+    List<string> resolutionPaths;
+
+    [OneTimeSetUp]
+    public void RunBeforeAllTests()


thanks for doing this - I do hope we can come up with something better long term though. I believe there is a task in the backlog for it.

One thing we explored in the past was pulling full dynamo builds directly from s3 - that worked pretty well. There was even a msbuild task for it.

Having dynamo (runtime) installed on the test machine + 3dParty tests pointing to that install path sounds like a good approach .
Maybe the 3dparty dev should make sure to install dynamocore on the build machine (not sure if we should do it as part of the test project build process)
But if we do want to provide this convenience, we could also include a script as part of the test nuget, that the user could use to download a specific version of dynamo (runtime) and set the proper paths in the test config file

This might be a good idea for our test framework too (in order to segregate test deps from the runtime), separate out all the tests in a new solution and all tests would have a resolver to point to the dynamo output folder

We do install dynamo runtime as part of tests in other places (e.g. GD repos).

mjkkirschner · 2023-12-12T20:57:18Z

src/SampleLibraryZeroTouch/Examples/TraceExample.cs

@@ -56,7 +56,7 @@ public static TraceExampleItem ByString(string description)

            TraceExampleItem item = null;

-            int id;
+            string id;


instead of changing these types, you could just serialize and parse as it's just an int, or illustrate serialization using system.text.json.

mjkkirschner · 2023-12-12T20:59:13Z

src/SampleLinter/SampleLinter.csproj

+        <PackageXml Include="$(OutputPath)SampleLinter_ExtensionDefinition.xml" />
+        <SettingsXml Include="$(OutputPath)LinterSettings.xml" />
+    </ItemGroup>
+    <Copy SourceFiles="@(PackageDll)" DestinationFolder="$(ProjectDir)..\..\dynamo_linter\Sample Linter\bin" />


whats the structure of this output path - does it live in the package output folder? Is it a separate package from the main samples one, or is it in the same package?

it is a separate dynamo_linter folder

These packages are not exactly published, are they? Are they simply there for testing in Dynamo?

I do not know if anyone has published these packages (manually ?). There is no automation for it.
From the looks of it, it is there just to provide an example of how to build a package/linter/extension.
I do not think we have an example for the publishing process

aparajit-pratap · 2023-12-14T18:30:18Z

.github/workflows/build_dynamo_samples_net8.0.yml

+      run: echo "::remove-matcher owner=csc::"
+    - name: Install dependencies for SampleIntegration
+      run: |
+        dotnet restore $Env:GITHUB_WORKSPACE\DynamoSamples\src\SampleIntegration\SampleIntegration.sln /p:Configuration=Release --runtime=win-x64 -p:DotNet=net8.0


is -p:DotNet=... the same as /p:DotNet=...?

Yes..but dotnet restore has a different format for properties than msbuild

sorry, why are you using both formats here?

We do not need the -p:DotNet=net8.0 here. Removed
Looks like dotnet restore requires /p: for supplying properties
--runtime=win-x64 is separate from properties.

.github/workflows/build_dynamo_samples_net8.0.yml

src/SampleIntegration/TracedHog.cs

aparajit-pratap · 2023-12-14T19:25:44Z

src/SampleLibraryTests/TestServices.dll.config

-    <add key="RequestedLibraryVersion" value="Version223"/>
-  </appSettings>
+    <appSettings>
+        <add key="DynamoBasePath" value="D:\dev\dynamo\DynamoDS\bin\AnyCPU\debug"/>


Obviously, this is not meant for CICD, is there CICD setup for this project?

aparajit-pratap · 2023-12-14T19:28:34Z

src/SampleLibraryUI/SampleLibraryUI.csproj

    <ItemGroup>
-      <PackageFiles Include="$(OutDir)\SampleLibraryUI.dll;$(OutDir)\SampleLibraryUI.XML;$(OutDir)\SampleLibraryUI.dll.config" />
+      <PackageFiles Include="$(OutDir)\SampleLibraryUI.dll;$(OutDir)\SampleLibraryUI.XML" />
      <ENResourceFiles Include="$(OutDir)\en-US\*.*" />
      <ESResourceFiles Include="$(OutDir)\es-ES\*.*" />
    </ItemGroup>


For some reason I'm having difficulty getting these afterbuild targets to run in Meshtoolkit

aparajit-pratap

Few minor comments, and some questions, then LGTM.

pinzart90 · 2024-01-03T04:04:48Z

Needs to consume Dynamo nugets after this PR goes in DynamoDS/Dynamo#14774

mjkkirschner · 2024-01-03T18:03:18Z

src/Config/CS.props

+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <PlatformTarget >x64</PlatformTarget>
+    <VisualStudioVersion Condition="'$(VisualStudioVersion)' == ''">16.0</VisualStudioVersion>


what is this for?

mjkkirschner · 2024-01-03T18:04:21Z

src/Config/CS.props

+    <VisualStudioVersion Condition="'$(VisualStudioVersion)' == ''">16.0</VisualStudioVersion>
+    <TargetFramework>net8.0</TargetFramework>
+    <FileAlignment>512</FileAlignment>
+    <RuntimeIdentifier>win-x64</RuntimeIdentifier>


is this needed for all projects? I guess it's not that important now but it would be nice if our ZT and extension samples could run on linux builds.

mjkkirschner · 2024-01-03T18:04:52Z

src/SampleExtension/SampleExtension.csproj

-    <TargetFrameworkVersion>v4.8</TargetFrameworkVersion>
-    <FileAlignment>512</FileAlignment>
-    <TargetFrameworkProfile />
+    <EnableDynamicLoading>true</EnableDynamicLoading>


because this is a sample and this is a pretty rare property, maybe add a comment?

mjkkirschner · 2024-01-03T18:06:11Z

src/SampleLibraryTests/SampleLibraryTests.csproj

-    <TargetFrameworkVersion>v4.8</TargetFrameworkVersion>
-    <FileAlignment>512</FileAlignment>
-    <TargetFrameworkProfile />
+    <TargetFramework>net8.0-windows</TargetFramework>


I think these are worth adding comments for so that sample users don't blindly add these to their repos if they don't need them.

mjkkirschner · 2024-01-03T18:06:40Z

src/SampleLibraryTests/SampleLibraryTests.csproj

-  <Target Name="AfterBuild">
-  </Target>
+  <ItemGroup>
+    <PackageReference Include="DynamoVisualProgramming.Tests" Version="1.0.0" />


1.0.0 ? Is this a local build?

yes...will change once I have the dynamo nugets

mjkkirschner · 2024-01-03T18:07:31Z

src/SampleLibraryUI/SampleLibraryUI.csproj

-    <WarningLevel>4</WarningLevel>
-    <DocumentationFile>bin\Release\$(UICulture)\SampleLibraryUI.XML</DocumentationFile>
-    <Prefer32Bit>false</Prefer32Bit>
+    <TargetFramework>net8.0-windows</TargetFramework>


yada yada, this is a UI project that references wpf so add these properties

mjkkirschner

still a few comments but looks good.

aparajit-pratap · 2024-01-10T21:18:36Z

src/SampleLibraryTests/Setup.cs

+    public void RunBeforeAllTests()
+    {
+        var thisDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location);
+        var configPath = Path.Combine(thisDir, "TestServices.dll.config");


So this config file is in the test nuget package that is a dependency of dynamosamples from the work done in DynamoDS/Dynamo#14774?

YEs. And ideally the content of this file (Setup.cs) would live in the SystemTestServices.dll (or somewhere in Dynamo). But I found it too complicated to do it now

aparajit-pratap

~~just one last question,~~ LGTM!

mjkkirschner · 2024-01-10T21:49:24Z

@pinzart90 looks like there are some build failures due to nunit API changes?

pinzart90 · 2024-01-10T21:51:27Z

@pinzart90 looks like there are some build failures due to nunit API changes?

Yeah, nunit should come with dynamotests. Waiting for the nuggets

pinzart added 3 commits December 11, 2023 10:03

update

0601ecd

update

d987164

update

ace1fc8

pinzart90 marked this pull request as ready for review December 12, 2023 14:31

update

2735322

pinzart90 requested review from mjkkirschner and QilongTang December 12, 2023 15:11

Create Setup.cs

289c2af

mjkkirschner reviewed Dec 12, 2023

View reviewed changes

.github/workflows/build_dynamo_samples_net8.0.yml Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 12, 2023

View reviewed changes

pinzart added 2 commits December 12, 2023 20:11

update trace

8ffe71c

Update PeriodicUpdateExample.cs

be1ceb3

pinzart90 requested a review from mjkkirschner December 13, 2023 02:02

aparajit-pratap reviewed Dec 14, 2023

View reviewed changes

.github/workflows/build_dynamo_samples_net8.0.yml Outdated Show resolved Hide resolved

aparajit-pratap reviewed Dec 14, 2023

View reviewed changes

src/SampleIntegration/TracedHog.cs Show resolved Hide resolved

aparajit-pratap reviewed Dec 14, 2023

View reviewed changes

aparajit-pratap approved these changes Dec 14, 2023

View reviewed changes

pinzart and others added 2 commits December 22, 2023 13:36

update

0875f6c

Update Setup.cs

397a25e

mjkkirschner reviewed Jan 3, 2024

View reviewed changes

mjkkirschner approved these changes Jan 3, 2024

View reviewed changes

update

d8c00b9

pinzart90 requested review from mjkkirschner and aparajit-pratap January 8, 2024 16:12

aparajit-pratap reviewed Jan 10, 2024

View reviewed changes

aparajit-pratap approved these changes Jan 10, 2024

View reviewed changes

update

c74dad8

pinzart90 merged commit 62c0c33 into master Jan 11, 2024
10 checks passed

pinzart90 deleted the update_3.0 branch January 11, 2024 21:04

update for the Dynamo 3.0 release #50

update for the Dynamo 3.0 release #50

Conversation

pinzart90 commented Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap left a comment

Choose a reason for hiding this comment

pinzart90 commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner left a comment

Choose a reason for hiding this comment

aparajit-pratap Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap left a comment • edited Loading

Choose a reason for hiding this comment

mjkkirschner commented Jan 10, 2024

pinzart90 commented Jan 10, 2024

pinzart90 commented Dec 11, 2023 •

edited

Loading

aparajit-pratap Jan 10, 2024 •

edited

Loading

aparajit-pratap left a comment •

edited

Loading