paket.exe uses GetCustomAttributes and loads dependencies, could it use GetCustomAttributesData? #1289

Closed
dsyme opened this Issue Dec 4, 2015 · 21 comments

Comments

Projects
None yet
4 participants
@dsyme
Contributor

dsyme commented Dec 4, 2015

I'm getting a failure in a paket pack when a dependencies of the DLLs being packed is missing in the target directory. The callstack is below, but the basic problem is that paket is using GetCustomAttributes on the assembly being packed. I think it would be better if it used ReflectionOnlyLoadFrom and GetCustomAttributesData, or Mono.Cecil, or some other way of looking at the custom attributes that doesn't involve loading types and dependencies.

    [Managed to Native Transition]  
>   mscorlib.dll!System.ModuleHandle.ResolveTypeHandleInternal(System.Reflection.RuntimeModule module, int typeToken, System.RuntimeTypeHandle[] typeInstantiationContext, System.RuntimeTypeHandle[] methodInstantiationContext)   Unknown
    mscorlib.dll!System.Reflection.RuntimeModule.ResolveType(int metadataToken, System.Type[] genericTypeArguments, System.Type[] genericMethodArguments)   Unknown
    mscorlib.dll!System.Reflection.CustomAttribute.FilterCustomAttributeRecord(System.Reflection.CustomAttributeRecord caRecord, System.Reflection.MetadataImport scope, ref System.Reflection.Assembly lastAptcaOkAssembly, System.Reflection.RuntimeModule decoratedModule, System.Reflection.MetadataToken decoratedToken, System.RuntimeType attributeFilterType, bool mustBeInheritable, object[] attributes, System.Collections.IList derivedAttributes, out System.RuntimeType attributeType, out System.IRuntimeMethodInfo ctor, out bool ctorHasParameters, out bool isVarArg) Unknown
    mscorlib.dll!System.Reflection.CustomAttribute.GetCustomAttributes(System.Reflection.RuntimeModule decoratedModule, int decoratedMetadataToken, int pcaCount, System.RuntimeType attributeFilterType, bool mustBeInheritable, System.Collections.IList derivedAttributes, bool isDecoratedTargetSecurityTransparent)    Unknown
    mscorlib.dll!System.Reflection.CustomAttribute.GetCustomAttributes(System.Reflection.RuntimeAssembly assembly, System.RuntimeType caType)   Unknown
    paket.exe!Paket.PackageMetaData.loadAssemblyAttributes(string fileName, System.Reflection.Assembly assembly)    Unknown
    paket.exe!Paket.PackageProcess.merge$cont@21(string buildConfig, string buildPlatform, Paket.ProjectFile projectFile, Paket.TemplateFile templateFile, Paket.TemplateFile withVersion, Microsoft.FSharp.Core.Unit unitVar)  Unknown
    paket.exe!Paket.PackageProcess.f@1-11(string buildConfig, string buildPlatform, Microsoft.FSharp.Core.FSharpOption<Paket.SemVerInfo> version, System.Collections.Generic.HashSet<string> allTemplateFiles, Paket.ProjectFile projectFile, Paket.TemplateFile templateFile)  Unknown
    paket.exe!Paket.PackageProcess.Pack<System.__Canon>(string workingDir, Paket.DependenciesFile dependencies, string packageOutputPath, Microsoft.FSharp.Core.FSharpOption<string> buildConfig, Microsoft.FSharp.Core.FSharpOption<string> buildPlatform, Microsoft.FSharp.Core.FSharpOption<string> version, Microsoft.FSharp.Core.FSharpOption<string> releaseNotes, Microsoft.FSharp.Core.FSharpOption<string> templateFile, Microsoft.FSharp.Core.FSharpOption<System.__Canon> excludedTemplates, bool lockDependencies, bool symbols)    Unknown
    paket.exe!Paket.Program.handler@350-20.Invoke(Nessos.Argu.ParseResults<Paket.Commands.PackArgs> results)    Unknown
    paket.exe!Paket.Program.processWithValidation<Paket.Commands.PackArgs>(Microsoft.FSharp.Core.FSharpFunc<Nessos.Argu.ParseResults<Paket.Commands.PackArgs>, bool> validateF, Microsoft.FSharp.Core.FSharpFunc<Nessos.Argu.ParseResults<Paket.Commands.PackArgs>, Microsoft.FSharp.Core.Unit> commandF, Paket.Commands.Command command, string[] args)    Unknown
    paket.exe!Paket.Program.processCommand@61-1<Paket.Commands.PackArgs>.Invoke(Paket.Commands.Command command, string[] args)  Unknown
    paket.exe!Paket.Program.main()  Unknown
    paket.exe!<StartupCode$paket>.$Paket.Program.main@()    Unknown
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

trying to make it work, but the change breaks some integration tests. so need to dig deeper

Member

forki commented Dec 4, 2015

trying to make it work, but the change breaks some integration tests. so need to dig deeper

dsyme added a commit to dsyme/FSharp.Control.AsyncSeq that referenced this issue Dec 4, 2015

dsyme added a commit to fsprojects/FSharp.Control.AsyncSeq that referenced this issue Dec 4, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

seems GetCustomAttributesData does not work together with ReflectionOnlyLoadFrom.
It works together with "LoadFrom". would that be enough?

Member

forki commented Dec 4, 2015

seems GetCustomAttributesData does not work together with ReflectionOnlyLoadFrom.
It works together with "LoadFrom". would that be enough?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Dec 4, 2015

Contributor

I'm not sure, probably. However it would definitely be better to avoid reflection-based loading altogether. BTW the same underlying issue as #645

I worked around the problem by making sure FSharp.Core is CopyLocal=true when building the library fsprojects/FSharp.Control.AsyncSeq#41

Contributor

dsyme commented Dec 4, 2015

I'm not sure, probably. However it would definitely be better to avoid reflection-based loading altogether. BTW the same underlying issue as #645

I worked around the problem by making sure FSharp.Core is CopyLocal=true when building the library fsprojects/FSharp.Control.AsyncSeq#41

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

However it would definitely be better to avoid reflection-based loading altogether.

yep we only want to have attributes. this is a never ending story.

Member

forki commented Dec 4, 2015

However it would definitely be better to avoid reflection-based loading altogether.

yep we only want to have attributes. this is a never ending story.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

see aaed3a4

we could also use "ReflectionOnlyLoadFrom + GetCustomAttributes" instead of "LoadFrom + GetCustomAttributesData". Not sure what makes more sense.

Member

forki commented Dec 4, 2015

see aaed3a4

we could also use "ReflectionOnlyLoadFrom + GetCustomAttributes" instead of "LoadFrom + GetCustomAttributesData". Not sure what makes more sense.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

anyways I hope the latest version works

Member

forki commented Dec 4, 2015

anyways I hope the latest version works

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Dec 4, 2015

Contributor

There is a lightweight assembly reader that fits in a single F# file here: https://github.com/fsharp/FSharp.Data/blob/master/src/CommonProviderImplementation/AssemblyReader.fs

It doesn't read IL but does read enough data to decode custom attributes, e.g. the decode routine is called here: https://github.com/fsharp/FSharp.Data/blob/0ea9937903d260

We could extract that to a separate repo and use paket single-file-include.

Contributor

dsyme commented Dec 4, 2015

There is a lightweight assembly reader that fits in a single F# file here: https://github.com/fsharp/FSharp.Data/blob/master/src/CommonProviderImplementation/AssemblyReader.fs

It doesn't read IL but does read enough data to decode custom attributes, e.g. the decode routine is called here: https://github.com/fsharp/FSharp.Data/blob/0ea9937903d260

We could extract that to a separate repo and use paket single-file-include.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Dec 4, 2015

Contributor

"lightweight" in the sense it doesn't depend on reflection and doesn't add a DLL

Contributor

dsyme commented Dec 4, 2015

"lightweight" in the sense it doesn't depend on reflection and doesn't add a DLL

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

sounds great. I guess I will just try to paket include it from there. no need to create a new repo

Member

forki commented Dec 4, 2015

sounds great. I guess I will just try to paket include it from there. no need to create a new repo

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

it seems to compile if I paket include the reader (see #1293)
is there a sample where I can steal the usage?

Member

forki commented Dec 4, 2015

it seems to compile if I paket include the reader (see #1293)
is there a sample where I can steal the usage?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 6, 2015

Member

I tried in https://github.com/fsprojects/Paket/pull/1293/files#diff-74c1bcac29cb19f058c1e6032cc1ce19R130, but the elements property is always an empty array. Is there something wrong in my code?

Member

forki commented Dec 6, 2015

I tried in https://github.com/fsprojects/Paket/pull/1293/files#diff-74c1bcac29cb19f058c1e6032cc1ce19R130, but the elements property is always an empty array. Is there something wrong in my code?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Dec 8, 2015

Contributor

You'll want the custom attributes on the assembly manifest contained within the IL module

Contributor

dsyme commented Dec 8, 2015

You'll want the custom attributes on the assembly manifest contained within the IL module

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 9, 2015

Member

that seems to work. thanks

Member

forki commented Dec 9, 2015

that seems to work. thanks

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 9, 2015

Member

this is now available in 2.36.0-alpha001 release.

@dsyme + @sergey-tihon please give it a try.

Member

forki commented Dec 9, 2015

this is now available in 2.36.0-alpha001 release.

@dsyme + @sergey-tihon please give it a try.

@sergey-tihon

This comment has been minimized.

Show comment
Hide comment
@sergey-tihon

sergey-tihon Dec 10, 2015

Member

Cool! 2.36.0-alpha001 works for me now (on SwaggerProvider project).
Thank you @forki !!!

👍 to move Assembly Reader to separate project

Member

sergey-tihon commented Dec 10, 2015

Cool! 2.36.0-alpha001 works for me now (on SwaggerProvider project).
Thank you @forki !!!

👍 to move Assembly Reader to separate project

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 10, 2015

Member

cool. released in 2.36

I wonder if we could use the same approach for FSharp.Formatting. Does this AssemblyReader allow to retrieve XMLDocs for all methods? //cc @tpetricek

Member

forki commented Dec 10, 2015

cool. released in 2.36

I wonder if we could use the same approach for FSharp.Formatting. Does this AssemblyReader allow to retrieve XMLDocs for all methods? //cc @tpetricek

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Dec 10, 2015

Contributor

@forki - No, it doesn't read XML, just the binary. Also it doesn't resolve references (e.g. to find out facts about a referenced type), which is normally needed.

Contributor

dsyme commented Dec 10, 2015

@forki - No, it doesn't read XML, just the binary. Also it doesn't resolve references (e.g. to find out facts about a referenced type), which is normally needed.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 11, 2015

Member

Ah that's a pity, but thanks.
On Dec 10, 2015 23:46, "Don Syme" notifications@github.com wrote:

@forki https://github.com/forki - No, it doesn't read XML, just the
binary


Reply to this email directly or view it on GitHub
#1289 (comment).

Member

forki commented Dec 11, 2015

Ah that's a pity, but thanks.
On Dec 10, 2015 23:46, "Don Syme" notifications@github.com wrote:

@forki https://github.com/forki - No, it doesn't read XML, just the
binary


Reply to this email directly or view it on GitHub
#1289 (comment).

@forki forki closed this Dec 12, 2015

@sergey-tihon

This comment has been minimized.

Show comment
Hide comment
@sergey-tihon

sergey-tihon Dec 12, 2015

Member

Interesting to know, Why we do not use Mono.Cecil?

Member

sergey-tihon commented Dec 12, 2015

Interesting to know, Why we do not use Mono.Cecil?

@tpetricek

This comment has been minimized.

Show comment
Hide comment
@tpetricek

tpetricek Dec 12, 2015

Member

For F# Formatting? We want to get F# specific info like modules and functions...

Member

tpetricek commented Dec 12, 2015

For F# Formatting? We want to get F# specific info like modules and functions...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment