Resolve assemblies from specific paths #567

sbomer · 2019-05-14T00:17:20Z

This introduces a command-line option, "-reference ", which makes it possible to give the linker full assembly paths to resolve.

LinkTask now uses "-reference" instead of "-d", to prevent the directory resolution behavior from finding dependencies next to inputs. Now it takes an explicit list of inputs, which will be the only files it resolves.

When passing along full paths to each file, the command-line becomes substantially longer. To prevent hitting command length limits, arguments are now passed via a response file (using ToolTask's GenerateResponseFileCommands).

This fixes issues like that mentioned in https://github.com/dotnet/coreclr/issues/24397#issuecomment-490807112 when trying to link WPF apps, which have a WindowsBase.dll in the core framework directory and also in the WPF runtime pack:

Unhandled Exception: System.ArgumentNullException: Value cannot be null.
         Parameter name: declaringType
            at Mono.Cecil.Mixin.CheckType(Object type, Argument argument)
            at Mono.Cecil.MethodReference..ctor(String name, TypeReference returnType, TypeReference declaringType)
            at Mono.Linker.Steps.TypeMapStep.CreateGenericInstanceCandidate(GenericContext context, TypeDefinition candidateType, MethodDefinition interfaceMethod)
            at Mono.Linker.Steps.TypeMapStep.GetBaseInflatedInterfaceMethodInTypeHierarchy(GenericContext context, TypeDefinition type, MethodDefinition interfaceMethod)
            at Mono.Linker.Steps.TypeMapStep.MapInterfaceMethodsInTypeHierarchy(TypeDefinition type)
            at Mono.Linker.Steps.TypeMapStep.MapType(TypeDefinition type)
            at Mono.Linker.Steps.TypeMapStep.ProcessAssembly(AssemblyDefinition assembly)
            at Mono.Linker.Steps.BaseStep.Process(LinkContext context)
            at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
            at Mono.Linker.Pipeline.Process(LinkContext context)
            at Mono.Linker.Driver.Run(ILogger customLogger)
            at Mono.Linker.Driver.Execute(String[] args, ILogger customLogger)
            at Mono.Linker.Driver.Main(String[] args)

sbomer · 2019-05-14T00:18:24Z

PTAL @swaroop-sridhar @fadimounir

This introduces a command-line option, "--ref <path>", which makes it possible to give the linker full assembly paths to resolve. LinkTask now uses "--ref" instead of "-d", to prevent the directory resolution behavior from finding dependencies next to inputs. Now it takes an explicit list of inputs, which will be the only files it resolves. When passing along full paths to each file, the command-line becomes substantially longer. To prevent hitting command length limits, arguments are now passed via a response file (using ToolTask's GenerateResponseFileCommands).

This is necessary for the '--ref' argument to work with the mono build of the linker. Due to the design of cecil's BaseAssemblyResolver, there is a small amount of duplication.

sbomer · 2019-05-15T00:52:09Z

@marek-safar PTAL.

swaroop-sridhar · 2019-05-16T00:18:36Z

src/ILLink.Tasks/LinkTask.cs

+			return args.ToString ();
+		}
+
+		protected override string GenerateResponseFileCommands ()


Can you please make the response file such that there is one argument/switch per line?
Basically add newlines at the end of each option?

This makes it easier to inspect the rsp files manually.

Done, thanks!

sbomer · 2019-05-22T18:30:01Z

Fixes #58.

marek-safar · 2019-05-23T10:34:47Z

src/linker/Linker/Driver.cs

@@ -240,6 +240,10 @@ public void Run (ILogger customLogger = null)
 								disabled_optimizations.Add (opt);

 							continue;
+
+						case "--ref":
+							context.Resolver.AddAssemblyPath (GetParam ());


I don't understand why we need a different name for something what -d <path> already covers

Something like --ref <path-to-file> is necessary to prevent issues with-d <path-to-directory> where different directories have dlls with the same name, but we only want to use one.

Specifically:
Our SDK passes the linker task a specific list of files as input. One of these is WindowsBase.dll from a netcoreapp directory. Some other inputs (for WPF apps) live in a windowsdesktop directory, which also contains a WindowsBase.dll (that we don't want to use). Before linking, the SDK already does conflict resolution to pick the right input. However, when the linker gets the directory of both inputs, it can resolve the wrong WindowsBase.dll when it scans whole directories.

Right so the intention is to --ref behave like
--reference /user/lib/file.dll? It would be good if the names in the code actually reflected that

Yup! Which names? Maybe SearchAssemblyPaths -> SearchFilePaths?

I'd start with the documentation, it should be
-reference <file> .....

similarly, with the resolver entry, replace AddAssemblyPath with AddReferenceAssembly or something like that

marek-safar · 2019-05-23T10:35:17Z

src/linker/Linker/AssemblyResolver.cs

+			return base.Resolve (name, parameters);
+		}
+
+		private AssemblyDefinition SearchAssemblyPaths (AssemblyNameReference name, IEnumerable<string> assemblyPaths, ReaderParameters parameters)


Why base implementation cannot be used?

The base classes currently only have logic to search in directories (BaseAssemblyResolver from cecil, or DirectoryAssemblyResolver from the linker). To move it to the base class, we would need to do one of:

Modify cecil's resolution behavior. I've attempted this in the past, but Jb suggested that the linker should own any resolution logic specific to its purposes.

Move it to DirectoryAssemblyResolver but not BaseAssemblyResolver, and support --ref only on illink, not on monolinker.

Keep it in the child class, extending both base classes.

I opted for the third approach, to keep the command-line consistent between illink/monolinker (I thought this option may be useful for monolinker as well, since it could in theory hit similar issues).

If you prefer, I can also make the change only for illink, and move the change to DirectoryAssemblyResolver only. This is actually what I did initially (see the first commit), before I realized that monolinker wouldn't support the change this way.

Please rename it the to ResolveFromReferences (AssemblyNameReference name, Collection<string> references, ReaderParameters parameters)

src/linker/Linker/AssemblyResolver.cs

marek-safar · 2019-05-23T18:50:05Z

src/linker/Linker/AssemblyResolver.cs

+			return base.Resolve (name, parameters);
+		}
+
+		private AssemblyDefinition SearchAssemblyPaths (AssemblyNameReference name, IEnumerable<string> assemblyPaths, ReaderParameters parameters)


Please rename it the to ResolveFromReferences (AssemblyNameReference name, Collection<string> references, ReaderParameters parameters)

marek-safar · 2019-05-23T18:51:51Z

src/linker/Linker/DirectoryAssemblyResolver.cs

@@ -50,7 +50,7 @@ public virtual AssemblyDefinition Resolve (AssemblyNameReference name, ReaderPar
 			if (name == null)


Aren't the ArgumentNullException check now redundant?

They're redundant in the sense that the base class performs the same check, but also necessary to prevent passing null to SearchAssemblyPaths (renamed to ResolveFromReferences).
You're right that we already deref name in Resolve, so maybe that's not strictly necessary, but I still feel that it's safer to keep both checks (moved to the beginning of Resolve).

--ref <path> -> --reference <file> SearchAssemblyPaths -> ResolveFromReferences AddAssemblyPath -> AddReferenceAssembly _assemblyPaths -> _references Also: - move parameter checking to beginning of Resolve () - remove unnecessary "System." - move computed string out of loop

sbomer · 2019-05-23T21:52:34Z

/azp help

azure-pipelines · 2019-05-23T21:52:39Z

Supported commands
     help:
          Get descriptions, examples and documentation about supported commands
          Example: help "command_name"
     list:
          List all pipelines for this repository using a comment.
          Example: "list"
     run:
          Run all pipelines or a specific pipeline for this repository using a comment. Use
          this command by itself to trigger all related pipelines, or specify a pipeline
          to run.
          Example: "run" or "run pipeline_name"
     where:
          Report back the Azure DevOps orgs that are related to this repository and org
          Example: "where"

See additional documentation.

sbomer · 2019-05-23T21:53:04Z

/azp run

azure-pipelines · 2019-05-23T21:53:15Z

Azure Pipelines successfully started running 1 pipeline(s).

* Resolve assemblies from specific paths This introduces a command-line option, "-reference <path>", which makes it possible to give the linker full assembly paths to resolve. LinkTask now uses "-reference" instead of "-d", to prevent the directory resolution behavior from finding dependencies next to inputs. Now it takes an explicit list of inputs, which will be the only files it resolves. When passing along full paths to each file, the command-line becomes substantially longer. To prevent hitting command length limits, arguments are now passed via a response file (using ToolTask's GenerateResponseFileCommands). * Move path resolution logic to child assembly resolver class This is necessary for the '--ref' argument to work with the mono build of the linker. Due to the design of cecil's BaseAssemblyResolver, there is a small amount of duplication. * Place response file arguments on separate lines * Move SearchAssemblyPaths call directly into Resolve * Update names for clarity and other PR feedback --ref <path> -> --reference <file> SearchAssemblyPaths -> ResolveFromReferences AddAssemblyPath -> AddReferenceAssembly _assemblyPaths -> _references Also: - move parameter checking to beginning of Resolve () - remove unnecessary "System." - move computed string out of loop * Update Driver.cs * Update Driver.cs * Update AssemblyResolver.cs * Update LinkTask.cs Commit migrated from dotnet/linker@923463b

sbomer requested a review from marek-safar as a code owner May 14, 2019 00:17

sbomer force-pushed the refInput branch from fe13c85 to 1bd01c0 Compare May 14, 2019 00:20

Move path resolution logic to child assembly resolver class

2074abc

This is necessary for the '--ref' argument to work with the mono build of the linker. Due to the design of cecil's BaseAssemblyResolver, there is a small amount of duplication.

swaroop-sridhar approved these changes May 16, 2019

View reviewed changes

Place response file arguments on separate lines

4ab145e

marek-safar requested changes May 23, 2019

View reviewed changes

Move SearchAssemblyPaths call directly into Resolve

b7d7504

sbomer requested a review from marek-safar May 23, 2019 17:07

marek-safar reviewed May 23, 2019

View reviewed changes

src/linker/Linker/AssemblyResolver.cs Outdated Show resolved Hide resolved

marek-safar reviewed May 23, 2019

View reviewed changes

sbomer requested a review from marek-safar May 23, 2019 22:14

marek-safar added 4 commits May 24, 2019 10:10

Update Driver.cs

cd9c2f2

Update Driver.cs

a757cd3

Update AssemblyResolver.cs

36f9c2c

Update LinkTask.cs

a0c7c63

marek-safar approved these changes May 24, 2019

View reviewed changes

marek-safar merged commit 923463b into dotnet:master May 24, 2019

ghost mentioned this pull request May 25, 2019

Use "-reference" in LinkTask #582

Merged

sbomer mentioned this pull request Sep 24, 2019

error MSB6006: "dotnet.exe" exited with code -532462766. (With PublishTrimmed=true) dotnet/sdk#3668

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve assemblies from specific paths #567

Resolve assemblies from specific paths #567

sbomer commented May 14, 2019 •

edited by marek-safar

Loading

sbomer commented May 14, 2019

sbomer commented May 15, 2019

swaroop-sridhar May 16, 2019

swaroop-sridhar May 16, 2019

sbomer May 16, 2019

sbomer commented May 22, 2019

marek-safar May 23, 2019

sbomer May 23, 2019 •

edited

Loading

marek-safar May 23, 2019

sbomer May 23, 2019

marek-safar May 23, 2019

sbomer May 23, 2019

marek-safar May 23, 2019

sbomer May 23, 2019

marek-safar May 23, 2019

marek-safar May 23, 2019

marek-safar May 23, 2019

sbomer May 23, 2019

sbomer commented May 23, 2019

azure-pipelines bot commented May 23, 2019

sbomer commented May 23, 2019

azure-pipelines bot commented May 23, 2019

		@@ -50,7 +50,7 @@ public virtual AssemblyDefinition Resolve (AssemblyNameReference name, ReaderPar
		if (name == null)

Resolve assemblies from specific paths #567

Resolve assemblies from specific paths #567

Conversation

sbomer commented May 14, 2019 • edited by marek-safar Loading

sbomer commented May 14, 2019

sbomer commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer commented May 22, 2019

Choose a reason for hiding this comment

sbomer May 23, 2019 • 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

sbomer commented May 23, 2019

azure-pipelines bot commented May 23, 2019

sbomer commented May 23, 2019

azure-pipelines bot commented May 23, 2019

sbomer commented May 14, 2019 •

edited by marek-safar

Loading

sbomer May 23, 2019 •

edited

Loading