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.

Sign up for GitHub

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

Jump to bottom

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

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

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

dsyme opened this issue Dec 4, 2015 · 21 comments
Labels
bug

Comments

@dsyme
Copy link
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
    [email protected](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 forki added the bug label Dec 4, 2015
@dsyme dsyme mentioned this issue Dec 4, 2015
Closed
@forki
Copy link
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
update build to workaround fsprojects/Paket#1289
6fb7bcf
dsyme added a commit to fsprojects/FSharp.Control.AsyncSeq that referenced this issue Dec 4, 2015
@dsyme
Merge pull request #41 from dsyme/fix-2
68b73bc 
update build to workaround fsprojects/Paket#1289
@forki
Copy link
Member

forki commented Dec 4, 2015

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

@dsyme
Copy link
Contributor Author

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
Copy link
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 added a commit that referenced this issue Dec 4, 2015
@forki
Use GetCustomAttributesData instead of GetCustomAttributes - references
aaed3a4 
#1289
@forki
Copy link
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
Copy link
Member

forki commented Dec 4, 2015

anyways I hope the latest version works

@dsyme
Copy link
Contributor Author

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
Copy link
Contributor Author

dsyme commented Dec 4, 2015

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

@forki
Copy link
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
Copy link
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?

@dsyme
Copy link
Contributor Author

dsyme commented Dec 5, 2015

This to create an ILModuleReader:

https://github.com/fsharp/FSharp.Data/blob/0ea9937903d26004322b3fc6f29863e721985b1a/src/CommonProviderImplementation/TypeProviderBindingContext.fs#L28

and this to decode attribute data:

https://github.com/fsharp/FSharp.Data/blob/0ea9937903d26004322b3fc6f29863e721985b1a/src/CommonProviderImplementation/AssemblyReaderReflection.fs#L482

@forki
Copy link
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
Copy link
Contributor Author

dsyme commented Dec 8, 2015

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

@forki
Copy link
Member

forki commented Dec 9, 2015

that seems to work. thanks

@forki
Copy link
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
Copy link
Member

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

👍 to move Assembly Reader to separate project

@forki
Copy link
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

@forki forki mentioned this issue Dec 10, 2015
Closed
@dsyme
Copy link
Contributor Author

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
Copy link
Member

forki commented Dec 11, 2015

Ah that's a pity, but thanks.
On Dec 10, 2015 23:46, "Don Syme" [email protected] 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 as completed Dec 12, 2015
@sergey-tihon
Copy link
Member

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

@tpetricek
Copy link
Member

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
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@forki @tpetricek @sergey-tihon @dsyme