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

Safeguards loading of assemblies during load script generation. #3098

Merged
merged 3 commits into from Feb 28, 2018

Conversation

Projects
None yet
3 participants
@ChrSteinert
Contributor

ChrSteinert commented Feb 28, 2018

This is a super naive approach to the problem of loading native assembies that will fail with a BadImageFormatException.

Fixes #3097 .
I confirmed, that this fixes the error for https://github.com/ChrSteinert/Paket-3097

@matthid

Catching everything is error prone

try
(AssemblyDefinition.ReadAssembly path, FileInfo(path)) |> Some
with
| _ -> None

This comment has been minimized.

@matthid

matthid Feb 28, 2018

Member

I don't like that pattern, can we at least catch only the relevant exception or otherwise log the error as warning?

try
(AssemblyDefinition.ReadAssembly (f.FullName:string), f) |> Some
with
| _ -> None)

This comment has been minimized.

@matthid

matthid Feb 28, 2018

Member

same here ;)

This comment has been minimized.

@ChrSteinert

This comment has been minimized.

@matthid

matthid Feb 28, 2018

Member

Yes, getting rid of them is hard, because you do not know what you have catched previously ;).
A PR or adding the other case is welcome as well ;)

@matthid

This comment has been minimized.

Member

matthid commented Feb 28, 2018

Thanks for taking care of this :)

@ChrSteinert

This comment has been minimized.

Contributor

ChrSteinert commented Feb 28, 2018

My pleasure!

@@ -98,7 +99,13 @@ type DependencyCache (lockFile:LockFile) =
installModel
|> InstallModel.getLegacyReferences (TargetProfile.SinglePlatform framework)
|> Seq.map (fun l -> l.Path)
|> Seq.map (fun path -> AssemblyDefinition.ReadAssembly path, FileInfo(path))
|> Seq.map (fun path ->

This comment has been minimized.

@forki

forki Feb 28, 2018

Member

wh not choose/pick here?

This comment has been minimized.

@matthid

matthid Feb 28, 2018

Member

One more question: What path is it here? Why is someone putting invalid references into the /lib folder?

This comment has been minimized.

@ChrSteinert

ChrSteinert Feb 28, 2018

Contributor

They are invalid in the sense that they are native x64 or x86 dlls that the assembly loader might not like to load.

And yes, the direct choose is obviously a better way.

@@ -121,7 +128,11 @@ type DependencyCache (lockFile:LockFile) =
let assemblyFilePerAssemblyDef =
libs |> Seq.map (fun (f:FileInfo) ->

This comment has been minimized.

@forki

forki Feb 28, 2018

Member

why not choose/pick directly here?

This comment has been minimized.

@ChrSteinert

ChrSteinert Feb 28, 2018

Contributor

Obviously here as well.

@forki forki merged commit 12c8c02 into fsprojects:master Feb 28, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@forki

This comment has been minimized.

Member

forki commented Feb 28, 2018

thanks

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