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

Include Mono.Cecil.Tests.csproj in netstandard build #444

Merged
merged 4 commits into from
Oct 24, 2017
Merged

Include Mono.Cecil.Tests.csproj in netstandard build #444

merged 4 commits into from
Oct 24, 2017

Conversation

tmat
Copy link
Contributor

@tmat tmat commented Sep 8, 2017 •
edited
Loading

Makes Mono.Cecil.Tests project portable targeting netstandard2.0 and tests runnable by dotnet test:

D:\cecil>dotnet test D:\cecil\Test\Mono.Cecil.Tests.csproj /p:Configuration=netstandard_debug
Build started, please wait...
Build completed.

Test run for D:\cecil\Test\bin\netstandard_debug\netcoreapp2.0\Mono.Cecil.Tests.dll(.NETCoreApp,Version=v2.0)
Microsoft (R) Test Execution Command Line Tool Version 15.3.0-preview-20170628-02
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
NUnit Adapter 3.8.0.0: Test execution started
Running all tests in D:\cecil\Test\bin\netstandard_debug\netcoreapp2.0\Mono.Cecil.Tests.dll
NUnit3TestExecutor converted 246 of 246 NUnit test cases
NUnit Adapter 3.8.0.0: Test execution complete

Total tests: 246. Passed: 246. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 31.0012 Seconds

@tmat
Copy link
Contributor Author

tmat commented Sep 8, 2017

@jbevain This is a part of PR #400, split to a separate PR as requested.

@tmat tmat force-pushed the NetStandardTests branch 2 times, most recently from 40d90fd to 8677d74 Compare September 8, 2017 23:36
jbevain
jbevain reviewed Sep 11, 2017
View reviewed changes
Mono.Cecil.Tests.props
</PropertyGroup>
<Import Project="Mono.Cecil.props" />
<ItemGroup>
<PropertyGroup Condition="'$(NetStandard)' == 'true'">
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need binding redirects?

Copy link
Contributor Author

@tmat tmat Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case we cross-target tests to also run on .NET Framework when building netstandard_debug configuration. I have removed the cross-targeting for now since it was breaking VS editing experience (VS project system doesn't like conditional cross-targeting).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmat so we can remove this and the workaround below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep it. It makes it easier to switch the target to net462 temporarily when debugging issues on .NET Framework. I'll follow up on the cross-targeting issue with Project System team.

jbevain
jbevain reviewed Sep 11, 2017
View reviewed changes
Mono.Cecil.props
</PropertyGroup>
<PropertyGroup Condition=" $(Configuration.StartsWith('net_4_0')) ">
<TargetFrameworkVersion>v4.0</TargetFrameworkVersion>
<DefineConstants>$(DefineConstants);NET_4_0;</DefineConstants>
</PropertyGroup>
<PropertyGroup Condition=" $(NetStandard) ">
<TargetFramework>netstandard1.3</TargetFramework>
<PropertyGroup Condition="'$(NetStandard)' == 'true'">
Copy link
Owner

@jbevain jbevain Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use $(NetStandard) and ! $(NetStandard)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most msbuild targets use "'$(Var)' == 'true'" to allow the variable to be empty. Empty string is not auto-converted to boolean false.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks!

jbevain
jbevain reviewed Sep 11, 2017
View reviewed changes
Test/Mono.Cecil.Tests/Extensions.cs Outdated
#else
public static SR.Assembly LoadAssembly (MemoryStream stream)
{
return SR.Assembly.Load(stream.ToArray ());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Load (

@jbevain
Copy link
Owner

jbevain commented Sep 11, 2017

Thanks for contributing this!

@jbevain
Copy link
Owner

jbevain commented Sep 11, 2017

Also let's move to net standard 2.0 and avoid the GetTypeInfo() everywhere.

@tmat
Copy link
Contributor Author

tmat commented Oct 9, 2017

Makes sense - i'll port to netstandard 2.0. I have made the change when 2.0 wasn't released yet.

@tmat tmat force-pushed the NetStandardTests branch from 8677d74 to 58a7dcf Compare October 10, 2017 03:03
@tmat
Copy link
Contributor Author

tmat commented Oct 10, 2017

@jbevain PR updated. Now targeting netstandard 2.0. All tests pass on Core CLR.

@tmat tmat force-pushed the NetStandardTests branch from 58a7dcf to 01886f0 Compare October 10, 2017 03:19
jbevain
jbevain reviewed Oct 10, 2017
View reviewed changes
Mono.Cecil.Cil/Symbols.cs Outdated
@@ -826,7 +826,7 @@ static SR.AssemblyName GetSymbolAssemblyName (SymbolKind kind)

var suffix = GetSymbolNamespace (kind);

var cecil_name = typeof (SymbolProvider).Assembly ().GetName ();
var cecil_name = typeof (SymbolProvider).GetTypeInfo ().Assembly.GetName ();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove all the GetTypeInfo() in favor of the existing API in netstd2?

Copy link
Contributor Author

@tmat tmat Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only tests target netstandard2.0. the product binary still targets netstandard1.3. I don't think we want to move to netstandard2.0 yet for the product binary

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because not everyone can move their apps to netstandard2.0. VS, for example, is on netstandard1.3 right now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, but do we actually have any tool that has a dependency on the netstd1.3 version of Cecil that can't move to 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know any specific one. Hard to say. I think it's better to be conservative. There is not that many of these APIs to be worth preventing people from using Cecil in their apps that can't move to 2.0 yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather consider removing the net35 and net40 targets ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I mean potentially in future, not in this PR).

@tmat
Copy link
Contributor Author

tmat commented Oct 16, 2017

@jbevain Any more feedback?

@jbevain
Copy link
Owner

jbevain commented Oct 16, 2017

@tmat yes: please do switch Mono.Cecil to be built on netstandard 2.0 in this PR. Any previous usage of 0.10 was in beta and I want to release 0.10 to target netstd 2.0. After that we'll clean up a few tidbits and we should be good to go. Thanks again for contributing this!

@tmat
Copy link
Contributor Author

tmat commented Oct 19, 2017 •
edited
Loading

@jbevain Since there are currently consumers who need this change when targeting net46, for which netstandard2.0 doesn't work (ILLinker) and also other PRs in progress that completely revamp the projects (e.g. #442) I think it would be best to keep netstandard1.3 for now and move to netstandard2.0 later, once the other PRs are merged. Otherwise we'll run into all sorts of issues. Either the build won't support net46 for a while or I'd need to update the configurations to cross-target and thus collide even more with #442 than I already do.

Sounds reasonable?

@jbevain
Copy link
Owner

jbevain commented Oct 19, 2017

Sounds like a plan. I'll look into merging this tonight. Thanks!

@tmat
Copy link
Contributor Author

tmat commented Oct 19, 2017

@jbevain Thanks!

@tmat
Copy link
Contributor Author

tmat commented Oct 22, 2017

@jbevain Gentle reminder :)

@jbevain jbevain changed the base branch from master to integrate-444 October 24, 2017 22:19
@jbevain jbevain changed the base branch from integrate-444 to master October 24, 2017 22:22
@jbevain jbevain merged commit 3145301 into jbevain:master Oct 24, 2017
@jbevain
Copy link
Owner

jbevain commented Oct 24, 2017

Thanks for your contribution!

@tmat
Copy link
Contributor Author

tmat commented Oct 25, 2017

@jbevain Thanks for feedback and reviews!

This was referenced Oct 25, 2017
Closed
Closed
@per-samuelsson per-samuelsson mentioned this pull request Feb 2, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbevain jbevain jbevain left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tmat @jbevain