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

[Linker] Remove bundled mscorlib.xml descriptor from mmp and mtouch #3772

Merged
merged 2 commits into from May 30, 2018
Merged

[Linker] Remove bundled mscorlib.xml descriptor from mmp and mtouch #3772

merged 2 commits into from May 30, 2018

Conversation

dalexsoto
Copy link
Member

@dalexsoto dalexsoto commented Mar 16, 2018 •
edited

This fixes #3749

We currently process both mscorlib.xml descriptors from the mtouch/mmp
bundle and also the one contained in mscorlib.dll as resource, we are now removing
the descriptor bundled inside mtouch and mmp in favour of the one bundled in
mscorlib.dll.

Source descriptors files for diff:

mono (master): https://github.com/mono/mono/blob/ea4274f2eb74cddf23c9c219b086cd41add0efce/mcs/class/corlib/LinkerDescriptor/mscorlib.xml
XI (master): https://github.com/xamarin/xamarin-macios/blob/da6db5f6601b74fbc8324e974c83a4c33186dc12/tools/linker/Descriptors/mscorlib.xml
XM (master): https://github.com/xamarin/xamarin-macios/blob/da6db5f6601b74fbc8324e974c83a4c33186dc12/tools/mmp/linker/Descriptors/mscorlib.xml

Diffs:

Mono - XI descriptor diff: https://gist.github.com/dalexsoto/f0b1c9c66bf50edf8198063ec039a17b
Mono - XM descriptor diff: https://gist.github.com/dalexsoto/06b253a6743d366a4b9addc21a1e2c2a

@dalexsoto dalexsoto added this to the d15-8 milestone Mar 16, 2018
@dalexsoto dalexsoto requested a review from chamons as a code owner March 16, 2018 17:40
@dalexsoto dalexsoto added run-mtouch-tests Run the mtouch tests run-mmp-tests Run the mmp tests labels Mar 16, 2018
chamons
chamons approved these changes Mar 16, 2018
View reviewed changes
Copy link
Contributor

@chamons chamons left a comment

Choose a reason for hiding this comment

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

I'm kinda surprised we didn't reference mscorlib.xml anywhere in code, but a quick grep didn't bring up anything obvious.

rolfbjarne
rolfbjarne reviewed Mar 16, 2018
View reviewed changes
tools/mmp/Makefile Outdated
@@ -104,7 +104,6 @@ tuner_sources = \
../../src/build/mac/Constants.cs

linker_resources = \
Copy link
Member

Choose a reason for hiding this comment

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

Can't you remove the entire linker_resources variable?

tools/mtouch/Makefile Outdated
@@ -15,7 +15,6 @@ MONOLINKER=$(LINKER_TOOLS_PATH)/linker
MONO_TUNER=$(LINKER_TOOLS_PATH)/tuner

LINKER_RESOURCES = \
Copy link
Member

Choose a reason for hiding this comment

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

Same: redundant variable now.

spouliot
spouliot suggested changes Mar 16, 2018
View reviewed changes
Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

let's remove both files too (from our repo)

@dalexsoto
Copy link
Member Author

@rolfbjarne / @spouliot done! Thanks for the feedback!

@spouliot spouliot added the requires-approval-before-merge The pull request requires special approval before it can be merged label Mar 16, 2018
rolfbjarne
rolfbjarne approved these changes Mar 16, 2018
View reviewed changes
Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Still pending results from the device tests, which will probably not complete until sometime next week.

@monojenkins
Copy link
Collaborator

Build failure

@dalexsoto dalexsoto added the do-not-merge Do not merge this pull request label Mar 20, 2018
@dalexsoto dalexsoto mentioned this pull request Mar 20, 2018
Closed
@dalexsoto
Copy link
Member Author

This PR is blocked on mono/mono#7716

@dalexsoto
Copy link
Member Author

dalexsoto commented Mar 21, 2018 •
edited

Two tests are failing:

BaseOptimizeGeneratedCodeTest_SetupBlockPerfTest- Known issue -> https://github.com/xamarin/maccore/issues/649

The second one is xammac_tests/Release (all optimizations) which is an issue with the xml descriptor missing this entry <namespace fullname="System.Security.Cryptography" feature="crypto" />, that entry seems not ideal so Marek will have a look.

Also this PR can only go in until the descriptor is fixed in mono and mono 2018-02 #3402 is merged and rebased with the mono fix.

@dalexsoto
[Linker] Remove bundled mscorlib.xml descriptor from mmp and mtouch
a79d7e2 
This fixes xamarin#3749

We currently process both mscorlib.xml descriptors[1] from the mtouch/mmp
bundle and also the one contained in mscorlib.dll as resource, we are now removing
the descriptor bundled inside mtouch and mmp in favour of the one bundled in
mscorlib.dll.

Source descriptors files for diff:

mono (master): https://github.com/mono/mono/blob/ea4274f2eb74cddf23c9c219b086cd41add0efce/mcs/class/corlib/LinkerDescriptor/mscorlib.xml
XI (master): https://github.com/xamarin/xamarin-macios/blob/da6db5f6601b74fbc8324e974c83a4c33186dc12/tools/linker/Descriptors/mscorlib.xml
XM (master): https://github.com/xamarin/xamarin-macios/blob/da6db5f6601b74fbc8324e974c83a4c33186dc12/tools/mmp/linker/Descriptors/mscorlib.xml

Diffs:

Mono - XI descriptor diff: https://gist.github.com/dalexsoto/f0b1c9c66bf50edf8198063ec039a17b
Mono - XM descriptor diff: https://gist.github.com/dalexsoto/06b253a6743d366a4b9addc21a1e2c2a

[1]: xamarin#3749 (comment)
@dalexsoto
Removed mscorlib.xml files and remove unused var in makefiles
f3c5235
@monojenkins
Copy link
Collaborator

Build success
Build comment file:

✅ Provisioning succeeded
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only)
✅ Generator Diff
✅ Test run succeeded

@spouliot spouliot removed do-not-merge Do not merge this pull request requires-approval-before-merge The pull request requires special approval before it can be merged labels May 30, 2018
@dalexsoto dalexsoto merged commit 070c271 into xamarin:master May 30, 2018
@dalexsoto dalexsoto deleted the GHIssue3749 branch May 30, 2018 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolfbjarne rolfbjarne rolfbjarne approved these changes

@spouliot spouliot spouliot approved these changes

@chamons chamons chamons approved these changes

@VincentDondain VincentDondain VincentDondain approved these changes

Assignees
No one assigned
Labels
run-mmp-tests Run the mmp tests run-mtouch-tests Run the mtouch tests
Projects
None yet
Milestone
d15-8
Development

Successfully merging this pull request may close these issues.

Remove Descriptors/mscorlib.xml
6 participants
@dalexsoto @monojenkins @rolfbjarne @spouliot @chamons @VincentDondain