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

Update ILLinkTasksVersion dependency #32170

Merged
merged 4 commits into from
Feb 18, 2020
Merged

Conversation

marek-safar
Copy link
Contributor

@marek-safar marek-safar commented Feb 12, 2020

This reverts commit 334391b and allows re-enabling linker darc subscription

@marek-safar marek-safar marked this pull request as ready for review February 14, 2020 22:24
@marek-safar marek-safar changed the title Revert "Revert "Update ILLinkTasksVersion dependency (#2334)" (#31807)" Update ILLinkTasksVersion dependency Feb 14, 2020
@ViktorHofer
Copy link
Member

@marek-safar I assume this also includes the fix to not strip out assembly attributes which caused our official builds to fail?

@@ -13,7 +13,7 @@
<_ILLinkTrimXmlFilePath Condition=" '$(_ILLinkTrimXmlFilePath)' == '' ">$(MSBuildThisFileDirectory)ILLinkTrim.xml</_ILLinkTrimXmlFilePath>
<_ILLinkTasksToolsDir>$(PkgILLink_Tasks)/tools</_ILLinkTasksToolsDir>
<_ILLinkTasksDir>$(_ILLinkTasksToolsDir)/$(NetFrameworkCurrent)/</_ILLinkTasksDir>
<_ILLinkTasksDir Condition="'$(MSBuildRuntimeType)' == 'Core'">$(_ILLinkTasksToolsDir)/netcoreapp2.0/</_ILLinkTasksDir>
<_ILLinkTasksDir Condition="'$(MSBuildRuntimeType)' == 'Core'">$(_ILLinkTasksToolsDir)/netcoreapp3.0/</_ILLinkTasksDir>
Copy link
Member

@ViktorHofer ViktorHofer Feb 16, 2020

Choose a reason for hiding this comment

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

We should instead remove the property that calculates the path to the illink directory and instead use the defined one in L27 that comes via the package: https://github.com/mono/linker/blob/e5d6b69677dfc0da19c5b2c96b3466e8d5016327/src/ILLink.Tasks/ILLink.Tasks.targets#L8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion but as this needs more work it should be done in different PR (tracking issue #32453)

Copy link
Member

Choose a reason for hiding this comment

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

works for me, thanks for opening the issue.

@marek-safar
Copy link
Contributor Author

I assume this also includes the fix to not strip out assembly attributes which caused our official builds to fail?

@ViktorHofer yes, the tests were enabled and would fail without the fix

@ViktorHofer
Copy link
Member

Failures are #32320

@ViktorHofer ViktorHofer merged commit a0ae661 into dotnet:master Feb 18, 2020
@ViktorHofer
Copy link
Member

I just re-enabled the subscription.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants