-
Notifications
You must be signed in to change notification settings - Fork 519
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
[WIP] removing Mono.Cecil #3223
base: master
Are you sure you want to change the base?
[WIP] removing Mono.Cecil #3223
Conversation
What issue are we going to solve here? |
Attempting to bypass these It seems more feasible to me as Mono.Cecil does some heavy lifting |
Honestly it's not a problem of mono cecil. It's probably the only dependency that never failed us :P |
You are right :) Yet the sub-set used here is really-really small, and removing it is likely much simpler than changing NUnit (which (or it's adapter) may or may not use Mono incorrectly, depending on PoV). |
My PoV is that NUnit is broken, you can technically separate test assemblies from host assemblies (we do this for fake 5 for similar reasons) it is just work to do... I don't really want to test with a framework which doesn't even ensure that the correct dependencies are loaded. Regarding this PR. Yes this is a quick fix to the problem and no I don't like quickfix. That said: I'm practical enough to take it (I might regret saying this because of fake, but we will see) |
EmpNOrbit |
Just to give context on why Mono.Cecil was chosen instead of reflection:
I don't know about System.Reflection.Metadata but I assume that it fixes above problems, in which case it is ok to replace Cecil with it. |
@matthid I completely agree with you – NUnit needs to be fixed or deprecated. This PR exists only to 1) give us time until either of the aforementioned happens, and 2) remove a big 3rdParty library dependency, regardless of it's qualities, as a new lightweight and built-in way to do the same seems to be available It still can be completely rejected – I opened it as idea for discussion, not fully verified to be feasible on all supported platforms It just felt convenient to do the refactoring (2) where it can alleviate the immediate issue (1) :) |
Question: If we merge this and NUnit changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I think we can do this, just a couple of questions added.
Also do you happen to know if System.Reflection.Metadata
is designed to be forward compatible (ie it reads newer framework versions)? Or will it just throw not-supported exceptions in a typical Microsoft style and we need to update this fast to not break the ecosystem?
@@ -322,7 +322,7 @@ Target "QuickIntegrationTests" (fun _ -> | |||
// -------------------------------------------------------------------------------------- | |||
// Build a NuGet package | |||
|
|||
let mergeLibs = ["paket.exe"; "Paket.Core.dll"; "FSharp.Core.dll"; "Newtonsoft.Json.dll"; "Argu.dll"; "Chessie.dll"; "Mono.Cecil.dll"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to ILMerge this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for current platforms as it's part of netstandard, but might be needed to support older platforms and/or resolve discussed version conflict/s
assemblyName.SetPublicKey(keyOrToken) | ||
else | ||
assemblyName.SetPublicKeyToken(keyOrToken) | ||
assemblyName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semicolon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still new to F# :) is this incorrect? I prefer using semicolon when using a variable like this, just to make it the return value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually we leave out semicolons ;)
let references = | ||
match AssemblyMetadata.getAssemblyReferences metadataName with | ||
| Trial.Pass list -> list | ||
| Trial.Warn(list, warn) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does Trial.Warn
come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chessie – it seemed convenient to use here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don't see it emitted, so this case can never happen, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK it can be produced in getAssemblyReferences
by the pass|fail –> collect
pipeline
list | ||
| Trial.Fail exns -> | ||
exns |> List.iter (fun ex -> ex |> string |> traceError) | ||
List.empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is List.empty
the correct thing to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not – should I just re-throw here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yes, it will be caught with an evil with _ -> None
below. We probably should add some logging there (at the very least in verbose mode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would get double-logged then -- but I think we can do a throw with inner exception here to keep the original info/stack and get it logged in the caller only
warn |> List.iter (fun ex -> ex |> string |> traceWarn) | ||
list | ||
| Trial.Fail exns -> | ||
exns |> List.iter (fun ex -> ex |> string |> traceError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the scenarios where this happens (traceError and traceWarn) we might need to restrict this to verbose mode later.
Any idea when this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should only happen on metadata errors, not really expected – it might be safely restricted to one-time warning or error though
AFAIK it's a raw ECMA-335 metadata reader, used by Roslyn itself – if all current platforms are supported, which needs to be verified, it's quite safe assume future ones will be as well. When NUnit switches as well – as this is a System (netstandard) assembly, deployed with newer versions of framework, I'd expect a single version to be loaded – if we hit a problem here, it will very likely affect other netcore/netstandard assemblies like System.Collections.Immutable as well. There are some known issues with its older version (https://github.com/dotnet/corefx/issues/19548) and they might result in the need to ILMerge this, incl. dependencies – but that may be required to resolve the aforementioned version conflict as well, and may be the safe way to go. |
I'll need some guidance to continue on this – do we have some steps to create Mono/NetCore testing environment? My attempts to copy the Travis run failed so far, missing the |
Please review and comment :)
Approaching the issue from opposite side - replacing
Mono.Cecil
by built-inSystem.Reflection.Metadata
.Likely incomplete and/or unusable on .NET < 4.6.1 and Xamarin, but that's not easy to verify.