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

Fix netstandard2 + .NET 4.6.2 #235

Merged
merged 8 commits into from Sep 4, 2018
Merged

Conversation

Alexx999
Copy link
Contributor

@Alexx999 Alexx999 commented Aug 30, 2018

netstandard2 support layer for .NET 4.6.1-4.7.1 abuses redirects - it has both chained redirects (System.Object is redirected from System.Runtime.dll to netstandard.dll to mscorlib.dll) and duplicated redirects (System.Data.* types are redirected to System.Data.dll by both System.Data.Common.dll and netstandard.dll).
First case ends up with Assembly.GetTypes() throwing due to missing types and second case ends up with startup crash due to duplicated types.

@Alexx999
Copy link
Contributor Author

I wonder if it's OK to switch AppVeyor to VS2017 image?

@Alexx999
Copy link
Contributor Author

Alexx999 commented Sep 2, 2018

Switching requires unit test fixes from the other PR anyways so that depends if you're going to accept it (in which case that goes first) or not (then I'll cherry-pick that into here).
Builds fine on my master with "Previous VS2017" setting

@gluck
Copy link
Owner

gluck commented Sep 3, 2018

This one LGTM, I would support merging it before the other, which is too big to be reviewed quickly.
Is the cherry-pick big or is it just this one ?

Thx !

@Alexx999
Copy link
Contributor Author

Alexx999 commented Sep 3, 2018

I've cherry-picked relevant pieces from my master - tests should be OK now.
Failure with VS2017 v15.8 looks like com.ullink.msbuild issue - namely, ProjectFileParser crashes due to some dependency conflict, however, I didn't have time yet to dig deeper.

The other PR consists mostly of abandoned 0.10 branch - I have no idea why it was abandoned in the first place (lack of time I guess?) because it's extremely close to actually working on 0.10 cecil (sans Win32 resource handling).

@gluck gluck merged commit 3f3371a into gluck:master Sep 4, 2018
@gluck
Copy link
Owner

gluck commented Sep 4, 2018

Thanks ! will look at the cecil 0.10 branch when I get some time, would be great if you could rebase it.
Win32 was the main issue preventing the unfork, good idea separating it out.

@Alexx999 Alexx999 deleted the fix-netstandard2 branch September 4, 2018 09:00
@Alexx999
Copy link
Contributor Author

Alexx999 commented Sep 4, 2018

OK, I've rebased the other pull request.

@0x53A
Copy link

0x53A commented Sep 14, 2018

Hi, I think I'm hitting these issues when trying to merge asp.net core assemblies.

Would it be possible to push a new release to nuget?

Thank you for this awesome project, really helpfull!

@Alexx999
Copy link
Contributor Author

Alexx999 commented Sep 15, 2018

@0x53A I'm using package id avostres.ilrepack as a stopgap solution - it includes this PR and the bigger 0.10 cecil PR

@timotei
Copy link
Collaborator

timotei commented Jan 3, 2019

@Alexx999 @gluck We should probably update the CHANGELOG.md also, so it's clear what changed in this version.

<AppDesignerFolder>Properties</AppDesignerFolder>
<RootNamespace>DotNet462NetStandard2</RootNamespace>
<AssemblyName>DotNet462NetStandard2</AssemblyName>
<TargetFrameworkVersion>v4.6.2</TargetFrameworkVersion>
Copy link
Collaborator

@timotei timotei Jan 3, 2019

Choose a reason for hiding this comment

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

Hmm, for .NET standard it should actually be

        <TargetFramework>netstandard2.0</TargetFramework>

No? Otherwise it's just .NET 4.6.2

Also, the structure of the project should be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this PR says it's about "netstandard2 support layer for .NET 4.6.1-4.7.1"

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

Successfully merging this pull request may close these issues.

None yet

4 participants