This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Add install-distribution-stripped
ClosedPublic

Authored by smeenai on Nov 30 2017, 3:47 PM.

Details

Summary

This is identical to the install-distribution target, except that it
strips the installed binaries.

Event Timeline

smeenai created this revision.Nov 30 2017, 3:47 PM
beanz added inline comments.Dec 1 2017, 9:50 AM
CMakeLists.txt
352

I don't think there is any reason for this to be gated on an option. Once your other patches adding stripped targets have fully gone through you should be able to always add this.

smeenai added inline comments.Dec 1 2017, 11:04 AM
CMakeLists.txt
352

Like I said in the description, I added the install-*-distribution targets for pretty much everything I could think of, but it wouldn't surprise me at all if I missed something, and I'd rather not break people's cmake configures because of an oversight on my part.

One alternative would be not have this option, and instead unconditionally try to create the install-distribution-stripped target, but give a warning (not an error) if any of the individual requisite install-*-stripped targets don't exist. It might be slightly noisy, but the warnings should be pretty easy to address, and eventually they can be upgraded to an error. I'd also still announce the change on llvm-dev, so people understand why they're suddenly getting these warnings during their configure. What do you think?

beanz added a reviewer: phosek.Dec 1 2017, 3:03 PM
beanz added a subscriber: phosek.
beanz added inline comments.
CMakeLists.txt
352

I think that the LLVM_DISTRIBUTION_COMPONENTS option is used infrequently enough that it probably is fine to have as an error by default and not gated by an option.

The only place I think you're missing install-*-stripped targets is the llvm/runtimes directory's extra targets. I've added @phosek who has done a lot of the work in that area. I may have time to take a stab at it this afternoon.

smeenai added inline comments.Dec 1 2017, 3:27 PM
CMakeLists.txt
352

libc++'s is awaiting review (https://reviews.llvm.org/D40680).

I'd missed the runtimes one. I can throw up a patch for that if you haven't started working on that already.

I'm happy to make this unconditional then.

smeenai updated this revision to Diff 125238.Dec 1 2017, 3:40 PM

Make unconditional

smeenai edited the summary of this revision. (Show Details)Dec 1 2017, 3:40 PM
phosek edited edge metadata.Dec 1 2017, 3:59 PM

I'd be happy to review the runtimes part. Also, how is this supposed to interact with LLVM_EXTERNALIZE_DEBUGINFO?

Also, how is this supposed to interact with LLVM_EXTERNALIZE_DEBUGINFO?

The two are pretty orthogonal IMO. LLVM_EXTERNALIZE_DEBUGINFO controls the stripping of binaries in the build tree; this controls the stripping of binaries in the install tree. Since the binaries in the build tree are what get installed, LLVM_EXTERNALIZE_DEBUGINFO would also result in the installed binaries being stripped, but kinda indirectly. LLVM_EXTERNALIZE_DEBUGINFO also doesn't strip global symbols (it performs strip -gx) whereas this does, and the difference can be substantial in some cases (e.g. on my clang build strip -s saves 3 MB on top of strip -gx).

D40927 should be the last remaining piece needed here.

compnerd accepted this revision.Dec 8 2017, 11:23 AM
This revision is now accepted and ready to land.Dec 8 2017, 11:23 AM
This revision was automatically updated to reflect the committed changes.