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.

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

The use of Cecil 0.10.0-beta6 clashes with systems under test using the Cecil 0.10.0 release #488

Closed
SteveGilham opened this issue Mar 21, 2018 · 19 comments

Comments

@SteveGilham
Copy link

SteveGilham commented Mar 21, 2018

If I have a system under test using the new Mono.Cecil 0.10.0 final release, then the presence of the beta version can lead to MethodMissingException being thrown by the system under test. This was originally reported as a Cecil issue #500, but proved eventually to be due to the presence of the test adapter in the test dependencies.

  • NUnit and NUnit3TestAdapter versions
PackageReference Include="NUnit" Version="3.10.1" />
PackageReference Include="NUnit3TestAdapter" Version="3.10.0" />
  • Visual Studio edition and full version number (see Help About)
    Community 15.6.3

  • A short repro, preferably attached or pointing to a git repo or gist
    https://gist.github.com/SteveGilham/c86315da2f003cced56d23c128f7c363

  • What .net platform and version is being targeted
    .net core 2.0

  • If TFS/VSTS issue, what version, hosted, on-premises, and what build task you see this in
    n/a

@jnm2
Copy link
Contributor

jnm2 commented Mar 22, 2018

Thank you for filing! This looks like the same issue as #366. Is that right?

@SteveGilham
Copy link
Author

Yes -- another manifestation of assembly mismatch when the assembly versions are the same, but the API isn't. I would be hopeful that this one could be resolved (at least until the next Cecil release cycle starts) by simply packaging the newly released 0.10.0 which is then as new as everything if not newer.

@jnm2
Copy link
Contributor

jnm2 commented Mar 22, 2018

@rprouse and @OsirisTerje, assuming short-term it's okay to force everyone to 0.10.0 instead of forcing everyone to 0.10.0-beta, would it make sense to release the adapter with 0.10.0 bundled inside even though the engine was not compiled against it? Or should the 0.10.0 dependency be done in the engine first?

@rprouse
Copy link
Member

rprouse commented Mar 22, 2018

I would prefer to update it in the engine first to be safe. It could be a point release for just the .NET Standard engine. It is still going to be a problem for anyone not on .NET Core though. The full framework version of the engine targets .NET 2.0, but Mono.Cecil dropped support for 2.0 so we can't update. The only thing that would work there is to update the engine and everything else in that project to .NET 3.5 which would be contentious.

@jnm2
Copy link
Contributor

jnm2 commented Mar 22, 2018

We could use a conditional PackageReference element to pull a different version for the net20 target.

Also though, if we dropped net20, the only practical effect is that net20 test projects need to switch to net35. The projects being tested can stay net20. The difference is it would execute net20 code against the net35 libraries rather than the net20 libraries. In this way we would be the same as VSTest.

@jnm2
Copy link
Contributor

jnm2 commented Mar 22, 2018

We could use a conditional PackageReference element to pull a different version for the net20 target.

Also though, if we dropped net20, the only practical effect is that net20 test projects need to switch to net35. The projects being tested can stay net20. The difference is it would execute net20 code against the net35 libraries rather than the net20 libraries. In this way we would be the same as VSTest. (For anyone using Mono, this is already happening.)

@SteveGilham
Copy link
Author

Let me throw another data point into the ring -- the same code (a unit test calling a library that invokes Cecil) that fails when using dotnet test runs quite happily when it's in the context of a classic .net framework project and it's invoked via the test adapter 3.10.0 used as as a Visual Studio extension. I'm assuming that in that case, there's AppDomain isolation protecting the system under test, which isn't there under .net core.

@jnm2
Copy link
Contributor

jnm2 commented Mar 22, 2018

@SteveGilham Yes, that is what I observed. Hmm, I wonder what it's possible to do on .NET Core with a custom AssemblyLoadContext...

@SimonCropp
Copy link

i suggest ILmerging cecil into the nunit runner

@bording
Copy link

bording commented Apr 12, 2018

@SimonCropp ILMerge/ILRepack don't play nicely with .NET Standard /.NET Core assemblies. The resulting assemblies often end up nonsensical references. For example, I've seen an mscorlib reference appear, which definitely shouldn't happen.

@SimonCropp
Copy link

@bording

ILMerge/ILRepack don't play nicely with .NET Standard /.NET Core assemblies. The resulting assemblies often end up nonsensical references. For example, I've seen an mscorlib reference appear, which definitely shouldn't happen.

i have seen that as well. but i have never seen that manifest as a problem at runtime.

The other alternative to merging would be a custom build of cecil with a diff strong name

@bording
Copy link

bording commented Apr 13, 2018

@SimonCropp

i have seen that as well. but i have never seen that manifest as a problem at runtime.

Perhaps not, but I still don't trust the output at that point.

The other alternative to merging would be a custom build of cecil with a diff strong name

@jnm2 This is an approach that I think would be worth investigating, and seems like it would be more readily achiabve compared to the options you detailed in #366 (comment).

@jnm2
Copy link
Contributor

jnm2 commented Apr 13, 2018

Wouldn't maintaining a custom low-level reader be potentially more bang for the buck than maintaining a build of the entirety of Cecil?

@bording
Copy link

bording commented Apr 13, 2018

Wouldn't maintaining a custom low-level reader be potentially more bang for the buck than maintaining a build of the entirety of Cecil?

In the long term, sure that could be worth doing, but if you're looking for a way to solve issues now, bringing in Cecil (say via a submodule, for example) and using that is something you could get done right now and fix the pain people are having.

@rprouse
Copy link
Member

rprouse commented Apr 13, 2018

We've been trying to avoid sub-modules, but I am starting to lean towards our own build of Mono.Cecil. We could fork the repo to our organization, create a branch with a new signing key and then as we update, publish our version to MyGet.org.

@viceice
Copy link

viceice commented Jun 21, 2018

It would be nice to have an update to cecil 0.10.0, wich is availabe for net35, net40 and netstandard1.3.

My current workaround is to use

    <ItemGroup Condition=" '$(TargetFramework)' != 'netstandard2.0' ">
        <PackageReference Include="Mono.Cecil" Version="0.9.6.4" />
    </ItemGroup>

    <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
        <PackageReference Include="Mono.Cecil" Version="0.10.0" />
    </ItemGroup>

@viceice
Copy link

viceice commented Jun 21, 2018

nunit/nunit-console#389

@jnm2
Copy link
Contributor

jnm2 commented Aug 26, 2018

Good news: I fixed this and added nupkg acceptance tests proving that it works! #541

@jnm2
Copy link
Contributor

jnm2 commented Sep 11, 2018

Closed in #541 .

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

No branches or pull requests

6 participants