Page MenuHomePhabricator

[libcxx] Capture configuration information when installing the libc++ headers
ClosedPublic

Authored by EricWF on Oct 2 2015, 7:19 PM.

Details

Summary

Hi all,

This patch is a successor to D11963. However it has changed dramatically and I felt it would be best to start a new review thread.

Please read the design documentation added in this patch for a description of how it works.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 36427.Oct 2 2015, 7:19 PM
EricWF retitled this revision from to [libcxx] Capture configuration information when installing the libc++ headers.
EricWF updated this object.
EricWF added reviewers: mclow.lists, danalbert, jroelofs.
EricWF added a subscriber: cfe-commits.
EricWF added a comment.Oct 2 2015, 7:26 PM

Oh, but don't review the design doc too closely. I'm still working on it.

Adding subscribes from the old patch to this one.

EricWF updated this revision to Diff 36457.Oct 3 2015, 11:05 PM

I removed all non-essential changes in order to make this easy and fast to review. This patch is blocking a number of people so I would like to have this reviewed soon.

jroelofs edited edge metadata.Oct 5 2015, 7:24 AM

Design doc looks good! I think it captures all the design constraints well, and the proposed solution satisfies those constraints as far as I am concerned.

Thumbs up from me... but I'll still defer the final 'LGTM' to @mclow.lists.

Looks great!

include/CMakeLists.txt
31

I don't think >> would work on windows.
Do you really need __generated_config to be created at build time (as opposed to configure time)? You could use file(read) and file(append) then.

EricWF added inline comments.Oct 6 2015, 1:24 PM
include/CMakeLists.txt
31

I would strongly prefer the file got generated at build time so that it contains any changes made in the source tree. Any other behavior is developer hostile and non-obvious. In order to keep the installed headers consistent we need to do this. Although I hope there is a better way to achieve this.

eugenis added inline comments.Oct 6 2015, 1:27 PM
include/CMakeLists.txt
31

Right, good point. Then you could go back to the approach in D11963 where you called cmake in a custom command with a small script that used file(*) commands.

EricWF added inline comments.Oct 6 2015, 2:14 PM
include/CMakeLists.txt
31

That approach had a problem because the cmake INSTALL command used to invoke the script doesn't take a "COMPONENT" argument which would break the "install-libcxx" rule. On windows we could do it with a call out to python or a shell script. Would that work?

rnk added a subscriber: rnk.Oct 6 2015, 2:30 PM
rnk added inline comments.
include/CMakeLists.txt
31

The >> operator should work on Windows. It's supported by cmd. However, cat generally isn't available. If you use 'type' in place of 'cat' it should work.

EricWF updated this revision to Diff 36682.Oct 6 2015, 4:46 PM
EricWF edited edge metadata.

Use type instead of cat on windows as suggested by @rnk. @eugenis does this address your concern?

Use type instead of cat on windows as suggested by @rnk. @eugenis does this address your concern?

Absolutely!
LGTM

EricWF added a comment.Oct 6 2015, 4:50 PM

Cool. I've reached out to some platform maintainers to make sure they don't see any problems. I'll commit this once I hear back.

vkalintiris added inline comments.
include/CMakeLists.txt
9

Kind of silly but I believe that the files used for auto-generated config headers in LLVM have the extesions .cmake and .in, for cmake and autoconf, respectively.

EricWF marked an inline comment as done.Oct 9 2015, 4:56 PM

@eugenis I'll commit this tuesday morning If I don't hear anything over the weekend (Monday is a holiday for me).
If you rebase the ABI patch to work with this I'll make sure to review it ASAP.

include/CMakeLists.txt
9

libc++ uses .cmake for CMake scripts and .in for auto-generated files and I think this is fine. libc++ doesn't support autoconf I'm not worried about it causing confusion.

Thanks. I'll upload the rebase patch on Monday.

include/CMakeLists.txt
38

I think you need to depend on config_site.in as well. Or on ${LIBCXX_BINARY_DIR}/config_site

EricWF marked an inline comment as done.Oct 9 2015, 5:56 PM
EricWF added inline comments.
include/CMakeLists.txt
38

I don't think we need it. __config_site is generated with configure_file, which runs at configuration time and re-runs whenever the input file is changed. So __config_site should always be up to date.

However I'll see if we can actually make the change. It seems clearer and safer.

EricWF updated this revision to Diff 37013.Oct 9 2015, 6:07 PM

Make the generated config dependent on the "__config_site" file. It probably didn't need this but it doesn't hurt to have.

EricWF accepted this revision.Oct 13 2015, 3:12 PM
EricWF added a reviewer: EricWF.

Maintainers from Apple and FreeBSD gave this the thumbs up.

This revision is now accepted and ready to land.Oct 13 2015, 3:12 PM
EricWF closed this revision.Oct 13 2015, 3:13 PM
emaste added a subscriber: emaste.Oct 14 2015, 11:52 AM