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.

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

AGD-1437 Can not preload a package with an apostrophe in its the folder path. #10685

Merged
merged 3 commits into from
May 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Engine/ProtoCore/Utils/FileUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ public static string GetDSFullPathName(string fileName, Options options = null)
var trimChars = new[] {'\n','\t','\r',' '};
fileName = fileName.Trim(trimChars);

// Fix file paths which include an apostrophe
fileName = fileName.Replace("\\'", "'");
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand it correctly this is replacing \' with ', but how would the test case reflect this fix? Maybe I missed something?


//1. First search at .exe module directory, in case files of the same name exists in the following directories.
// The .exe module directory is of highest priority.
// CodeBase is used here because Assembly.Location does not work quite well when the module is shallow-copied in nunit test.
Expand Down
28 changes: 27 additions & 1 deletion test/DynamoCoreTests/DSLibraryTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public void TestLoadDllFileFailure()

[Test]
[Category("UnitTests")]
public void TestLoadDllFileSuccess()
public void TestLoadDllFileSuccessWithLibrarySearchPathsResultionToFindDLL()
{
LibraryLoaded = false;

Expand All @@ -121,6 +121,32 @@ public void TestLoadDllFileSuccess()
Assert.IsTrue(LibraryLoaded);
}

[Test]
[Category("UnitTests")]
public void TestLoadDllFileAtPathSuccess()
{
LibraryLoaded = false;

string libraryPath = Path.Combine(TestDirectory, "pkgs", "Package", "bin", @"Package.dll");

libraryServices.ImportLibrary(libraryPath);

Assert.IsTrue(LibraryLoaded);
}

[Test]
[Category("UnitTests")]
public void TestLoadDllFileWithApostropheInPathSuccess()
{
LibraryLoaded = false;

string libraryPath = Path.Combine(TestDirectory, "pkgs", "Pack'age", "bin", @"Package.dll");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang This test fails with out the formatting fix

Copy link
Contributor

Choose a reason for hiding this comment

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

@saintentropy maybe I misunderstand the issue, how would fileName.Replace("\\'", "'") help this case?

Copy link
Contributor Author

@saintentropy saintentropy May 21, 2020

Choose a reason for hiding this comment

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

Sorry now I get your question @QilongTang. The path here gets modified here

https://github.com/DynamoDS/Dynamo/blob/master/src/Engine/ProtoCore/Utils/CompilerUtils.cs#L258 in TryLoadAssemblyIntoCore

The return value of ToLiteral is modified from "Pack'age" to "Pack\\'age" when it is parsed here https://github.com/DynamoDS/Dynamo/blob/master/src/Engine/ProtoCore/Utils/CompilerUtils.cs#L242

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above is one example of the of the call to ToLiteral


libraryServices.ImportLibrary(libraryPath);

Assert.IsTrue(LibraryLoaded);
}

[Test]
[Category("UnitTests")]
public void TestLibraryAcrossSessions()
Expand Down
Binary file added test/pkgs/Pack'age/bin/Package.dll
Binary file not shown.
1 change: 1 addition & 0 deletions test/pkgs/Pack'age/pkg.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"license":"","file_hash":null,"name":"Package","version":"1.0.0","description":"original package","group":"","keywords":null,"dependencies":[],"contents":"","engine_version":"2.1.0.7840","engine":"dynamo","engine_metadata":"","site_url":"","repository_url":"","contains_binaries":true,"node_libraries":["Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"]}