DYN-5405 npm pack dependencies #13599

filipeotero · 2022-12-05T13:48:24Z

Purpose

This PR is to avoid installing all the npm dependencies of the Notification Center and Splash Screen.

Use of npm pack instead of npm install
Extract the downloaded .tgz file
Update the assembly path of the files
Update the .gitignore for the new files

Declarations

Check these if you believe they are true

The codebase 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.
This PR modifies some build requirements and the readme is updated

Reviewers

@QilongTang

FYIs

@RobertGlobant20

src/DynamoCoreWpf/DynamoCoreWpf.csproj

filipeotero · 2022-12-05T13:52:02Z

.gitignore

@@ -104,11 +104,7 @@ test/core/customast/test.txt
 test/core/files/test.png
 test/core/migration/writetext.txt
 logo.png
-/src/Notifications/node_modules


Removing the old folder hierarchy

filipeotero · 2022-12-05T13:52:33Z

src/DynamoCoreWpf/DynamoCoreWpf.csproj

@@ -14,16 +14,33 @@
  </PropertyGroup>

  <Target Name="NpmRunBuild" BeforeTargets="BeforeBuild">
-      <Exec Command="npm install --prefix ./ @dynamods/splash-screen@latest" />
+      <Exec Command="npm pack @dynamods/splash-screen@latest" />


The first step is to download the .tgz file

Would you add inline comments, you should be able to use 

Should we clean up none latest versioned npm package?

filipeotero · 2022-12-05T13:53:14Z

src/DynamoCoreWpf/DynamoCoreWpf.csproj

+
+    <Target Name="ExtractTGZFile" BeforeTargets="BeforeBuild">
+        <ItemGroup>
+            <TGZFiles Include="./*.tgz" />


This gets all the files that contain the .tgz suffix

filipeotero · 2022-12-05T13:54:24Z

src/DynamoCoreWpf/DynamoCoreWpf.csproj

+        </ItemGroup>
+
+        <ItemGroup>
+            <Reversed Include="@(TGZFiles->Reverse())" />


Reverts the order of the found files

filipeotero · 2022-12-05T13:54:37Z

src/DynamoCoreWpf/DynamoCoreWpf.csproj

+        </ItemGroup>
+
+        <PropertyGroup>
+            <Last>%(TGZFiles.Identity)</Last>


Gets the last one

filipeotero · 2022-12-05T13:54:55Z

src/DynamoCoreWpf/DynamoCoreWpf.csproj

+            <Last>%(TGZFiles.Identity)</Last>
+        </PropertyGroup>
+
+        <Exec Command="tar -xzf $(Last)"></Exec>


Extracts the selected file

QilongTang

No doubt this would work, but I have some concern.

If we want to specify the npm package version, similar to versioned Nuget package. If we stick to latest, it seems hard to get the exact package
If we want to clean up the legacy npm package.

Just two thoughts I would like team to think over

filipeotero · 2022-12-05T18:09:25Z

No doubt this would work, but I have some concern.

If we want to specify the npm package version, similar to versioned Nuget package. If we stick to latest, it seems hard to get the exact package

If we want to clean up the legacy npm package.

Just two thoughts I would like team to think over

1 - If we delete it after being extracted, maybe it won't be an issue since we will not maintain the files locally.

reddyashish · 2022-12-07T13:31:22Z

The self-service job is running in a loop and the build is failing. Converting this to draft to unblock the jobs.

filipeotero · 2022-12-07T21:35:23Z

We are getting this message from the self-service job:

18:27:12    tar -xzf c:\WorkspaceDynamo\src\DynamoCoreWpf\.\dynamods-splash-screen-1.0.5.tgz
18:27:13    'tar' is not recognized as an internal or external command,
18:27:13    operable program or batch file.
18:27:13  c:\WorkspaceDynamo\src\DynamoCoreWpf\DynamoCoreWpf.csproj(38,9): error MSB3073: The command "tar -xzf c:\WorkspaceDynamo\src\DynamoCoreWpf\.\dynamods-splash-screen-1.0.5.tgz" exited with code 9009.

Looks like there is no libarchive or something else to extract the tgz file.
Any ideas on how we can extract the file?

@QilongTang @reddyashish @RobertGlobant20

QilongTang · 2022-12-08T04:26:18Z

@filipeotero Should we include this dll in the repo, is that enought to leverage it?

mjkkirschner · 2022-12-14T21:30:14Z

@filipeotero @avidit (not sure what investigation you did of the builder image) but
two thoughts:

I see that the above document that @filipeotero linked says:
You can call these tools from servercore images as well. - I believe Abi verified tar was present - so yep thats confusing.

In addition, have you considered using powershell? (which we know works on the builder image)
https://scatteredcode.net/download-and-extract-gzip-tar-with-powershell/

🤦 ... the end of that post, if I had read far enough uses a 7zip powershell commandlet.

mjkkirschner · 2022-12-14T21:45:27Z

one more thought - if 7zip turns out to be the best solution, another approach is to add it to the builder image - this is nice because we add it for all our jobs that use that image, not just dynamo, in addition it makes it very unlikely we would distribute it - worth checking with legal if that makes things simpler.

filipeotero · 2022-12-15T14:05:39Z

Hi @mjkkirschner, I'm using VS 2019.
What about using the nuget package as you mentioned? I didn't try it yet but if it works we won't need to include binaries, right?

mjkkirschner · 2022-12-15T14:51:21Z

we don't need to include the binaries, but we still need to reference it and still need to go through legal.

avidit · 2022-12-15T15:14:56Z

@avidit (not sure what investigation you did of the builder image)

@mjkkirschner I ran docker run -it --rm mcr.microsoft.com/powershell:lts-windowsservercore-1909 tar --version and the output was bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.5.f-ipp

filipeotero · 2022-12-15T17:19:54Z

@mjkkirschner I submitted a new commit that deletes the binaries and references the nuget package. Also, I'm still trying to reach the legal team about using 7zip. Thanks!

filipeotero · 2023-01-11T14:51:42Z

@QilongTang Just to let you know, we have to wait for the EngOps to catch up before we merge this. https://jira.autodesk.com/browse/ESC-34251

QilongTang · 2023-01-11T16:17:52Z

src/DynamoCoreWpf/DynamoCoreWpf.csproj

-    <EmbeddedResource Include="node_modules\@dynamods\splash-screen\build\index.bundle.js" />
-    <EmbeddedResource Include="node_modules\@dynamods\splash-screen\build\index.html" />
+    <EmbeddedResource Include="package\build\index.bundle.js" />
+    <EmbeddedResource Include="package\build\index.html" />


If there is no need for a specific package name, to me it means that we would only support a single npm package per project. Do we need to specify the folder name during unzipping? I would like to think about if a project depending on two or more npm package, does the current structure work in that setup?

Is not necessary to specify the folder name during unzipping. By default, the folder created is called package. We can create folders named properly using the --directory argument from tar.

@filipeotero Understood, my point is that if in the future DynamoCoreWpf is depending on multiple npm packages, they will all be unzipped to the package folder and the index.html will be overwriting each other. If possible, I would love to have the dir specified to prevent that case from happening, what do you think?

Makes sense. I will do that!

QilongTang

LGTM but with one concern commented

sm6srw · 2023-02-02T13:29:26Z

@filipeotero I think we can get this PR ready for merging again. The BRE pipeline have been using VS2022 now for a couple of days.

filipeotero · 2023-02-02T14:34:43Z

@zeusongit Just to let you know, this PR is ready to be merged after the checks are completed. Thanks!

filipeotero added 6 commits December 1, 2022 18:14

build and extract files

b5c7f5b

splash screen config

c076159

build files notification center

6bda01b

Merge branch 'master' into DYN-5405-NpmPack-Dependencies

a6db514

Files configuration

3d09dbf

remove test code

97429db

filipeotero changed the title ~~Dyn 5405 npm pack dependencies~~ DYN-5405 npm pack dependencies Dec 5, 2022

QilongTang reviewed Dec 5, 2022

View reviewed changes

src/DynamoCoreWpf/DynamoCoreWpf.csproj Outdated Show resolved Hide resolved

QilongTang reviewed Dec 5, 2022

View reviewed changes

src/DynamoCoreWpf/DynamoCoreWpf.csproj Outdated Show resolved Hide resolved

filipeotero commented Dec 5, 2022

View reviewed changes

adding comments

48ce289

QilongTang reviewed Dec 5, 2022

View reviewed changes

removing the the file after extracting

8a2989a

retrigger checks

bd7d9ad

QilongTang approved these changes Dec 6, 2022

View reviewed changes

reddyashish marked this pull request as draft December 7, 2022 13:31

filipeotero added 2 commits December 7, 2022 15:11

Inserting depends on target property

0afb7c2

full file path

471b449

Adding commands to test self-service job

84a494e

filipeotero marked this pull request as ready for review December 12, 2022 14:33

filipeotero marked this pull request as draft December 12, 2022 15:05

avidit closed this Dec 15, 2022

avidit reopened this Dec 15, 2022

Removing 7zip binaries and adding nuget package

a71d7a9

filipeotero added 8 commits December 16, 2022 10:49

show executables from system 32

1553821

print hostnamo and %path%

a4264d5

printing dir \a

6ca6ec6

printing sysnative folder

2fa4149

set PreferredToolArchitecture to x64

0f5f464

testing tar again

7545c4a

update html string

12d1979

Removing 7zip from ignore file

8b88494

filipeotero requested review from mjkkirschner and QilongTang January 11, 2023 13:41

QilongTang reviewed Jan 11, 2023

View reviewed changes

filipeotero added 3 commits January 12, 2023 11:24

adding subfolders depending on the name of the packages

6138bd2

fix unit test

3280714

retrigger checks

a76c2e5

filipeotero added 2 commits February 2, 2023 10:58

Merge branch 'master' into DYN-5405-NpmPack-Dependencies

2199c34

merge from master

b41bc1f

zeusongit merged commit 2cec940 into DynamoDS:master Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-5405 npm pack dependencies #13599

DYN-5405 npm pack dependencies #13599

filipeotero commented Dec 5, 2022 •

edited

Loading

filipeotero Dec 5, 2022

filipeotero Dec 5, 2022

QilongTang Dec 5, 2022

QilongTang Dec 5, 2022

filipeotero Dec 5, 2022

filipeotero Dec 5, 2022

filipeotero Dec 5, 2022

filipeotero Dec 5, 2022

QilongTang left a comment •

edited

Loading

filipeotero commented Dec 5, 2022

reddyashish commented Dec 7, 2022

filipeotero commented Dec 7, 2022

QilongTang commented Dec 8, 2022

mjkkirschner commented Dec 14, 2022 •

edited

Loading

mjkkirschner commented Dec 14, 2022

filipeotero commented Dec 15, 2022

mjkkirschner commented Dec 15, 2022

avidit commented Dec 15, 2022 •

edited

Loading

filipeotero commented Dec 15, 2022

filipeotero commented Jan 11, 2023

QilongTang Jan 11, 2023 •

edited

Loading

filipeotero Jan 11, 2023

QilongTang Jan 12, 2023

filipeotero Jan 12, 2023

QilongTang left a comment

sm6srw commented Feb 2, 2023

filipeotero commented Feb 2, 2023

DYN-5405 npm pack dependencies #13599

DYN-5405 npm pack dependencies #13599

Conversation

filipeotero commented Dec 5, 2022 • edited Loading

Purpose

Declarations

Reviewers

FYIs

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

QilongTang left a comment • edited Loading

Choose a reason for hiding this comment

filipeotero commented Dec 5, 2022

reddyashish commented Dec 7, 2022

filipeotero commented Dec 7, 2022

QilongTang commented Dec 8, 2022

mjkkirschner commented Dec 14, 2022 • edited Loading

mjkkirschner commented Dec 14, 2022

filipeotero commented Dec 15, 2022

mjkkirschner commented Dec 15, 2022

avidit commented Dec 15, 2022 • edited Loading

filipeotero commented Dec 15, 2022

filipeotero commented Jan 11, 2023

QilongTang Jan 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

QilongTang left a comment

Choose a reason for hiding this comment

sm6srw commented Feb 2, 2023

filipeotero commented Feb 2, 2023

filipeotero commented Dec 5, 2022 •

edited

Loading

QilongTang left a comment •

edited

Loading

mjkkirschner commented Dec 14, 2022 •

edited

Loading

avidit commented Dec 15, 2022 •

edited

Loading

QilongTang Jan 11, 2023 •

edited

Loading