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

Revert "Use llvm-objcopy on Linux x64 (#48444)" #49092

Merged
1 commit merged into from
Mar 4, 2021
Merged

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Mar 3, 2021

This reverts commit ba50840.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas jkotas mentioned this pull request Mar 3, 2021
@sebastienros
Copy link
Member

Would there be any doubt and someone can build the impacted files for me then I can verify the change.

@jkotas
Copy link
Member Author

jkotas commented Mar 3, 2021

I will check the build artifacts once the build is green.

@jkotas
Copy link
Member Author

jkotas commented Mar 3, 2021

cc @am11

@am11
Copy link
Member

am11 commented Mar 3, 2021

It is one of those things, you can't verify locally. e.g. before and after my commit, as well as the master branch, the size of libcoreclr.so on linux-x64 is 6.8M.

My guess is that we need to check the dotnet/installer build if we are really using llvm v9x or higher there for linux-x64.

@jkotas
Copy link
Member Author

jkotas commented Mar 3, 2021

It is one of those things, you can't verify locally. e.g. before and after my commit, as well as the master branch, the size of libcoreclr.so on linux-x64 is 6.8M.

You should be able to use the docker image used by the CI to exactly replicate what the build does:

mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-20201227183837-5fe0e50

I do not think that the problem is in dotnet/installer. I see that the dotnet/runtime build is not stripping the .so as before. Let me know if you have are not able to replicate the problem using the docker image above.

@jkotas
Copy link
Member Author

jkotas commented Mar 4, 2021

I have built current main and this PR on mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-20201227183837-5fe0e50 docker image locally to verify that the problem can be reproduced:

Current main:

[root@3ffd9021d6f4 runtime]# cd /runtime/artifacts/bin/coreclr/Linux.x64.Release
[root@3ffd9021d6f4 Linux.x64.Release]# ls -la libcoreclr*
-rwxr-xr-x 1 root root  99492216 Mar  3 23:36 libcoreclr.so
-rw-r--r-- 1 root root 101239976 Mar  3 23:36 libcoreclr.so.dbg

With this revert:

[root@53b120e70d17 Linux.x64.Release]# ls -la libcoreclr*
-rwxr-xr-x 1 root root  7183624 Mar  3 23:38 libcoreclr.so
-rw-r--r-- 1 root root 94112016 Mar  3 23:38 libcoreclr.so.dbg

@ghost
Copy link

ghost commented Mar 4, 2021

Hello @jkotas!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@am11
Copy link
Member

am11 commented Mar 4, 2021

I haven't tried on that centos docker. I used Ubuntu 20.04 VM for my findings.

$ du -b artifacts/bin/coreclr/Linux.x64.Release/libcoreclr.so
7046336 artifacts/bin/coreclr/Linux.x64.Release/libcoreclr.so

@ghost ghost merged commit a324536 into dotnet:main Mar 4, 2021
@jkotas
Copy link
Member Author

jkotas commented Mar 4, 2021

@am11 Could you please take a look and re-submit the change with the fix for the size regression? Thank you!

@jkotas jkotas deleted the revert-484444 branch March 4, 2021 02:14
@ghost ghost locked as resolved and limited conversation to collaborators Apr 3, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants