- User Since
- Aug 19 2016, 10:21 AM (148 w, 4 d)
I'll wait for a day in case any other reviewers have comments and then commit this for you.
Looking good, just a few style nits.
Fri, Jun 21
Tue, Jun 18
Fri, Jun 14
Wed, Jun 12
Ah, so just having variables to hold the different distribution sets? Makes sense ... you'd need to adjust the CMake exporting logic, and I'm not quite sure how that'll play out yet, but we can see when we get to that.
I'll go ahead and commit this since all comments have been addressed.
Mon, Jun 10
Sorry, I thought James was gonna commit this, but I'll go ahead and do so.
Fri, Jun 7
Thu, Jun 6
This is technically a behavior change because with the old code, you wouldn't run any of the distribution code if LLVM_DISTRIBUTION_COMPONENTS was empty, even if LLVM_RUNTIME_DISTRIBUTION_COMPONENTS was non-empty. The new behavior seems more correct though.
This looks good, but what were you envisioning the interface would look like for configuring multiple distributions? It'd be a lot easier to judge with that context.
Wed, Jun 5
Tue, Jun 4
Phabricator memes are the best thing about Phabricator :)
Mon, Jun 3
Fri, May 31
Thu, May 30
LGTM, but I'd wait for @mtrent.
We're actively working on llvm-lipo (we have a bunch more diffs in the pipeline that'll be coming through over the summer). Is it worth adding either a TODO or filing a bug to change these tests once llvm-lipo is complete enough to support the required functionality?
LGTM with the NO_CMAKE_FIND_ROOT_PATH added.
Wed, May 29
Tue, May 28
May 24 2019
@winksaville I can commit this for you on Monday if no one's gotten to it before.
May 23 2019
May 20 2019
(or if they're different, what the difference is and whether that's desirable)
Could you perform a configure before and after this change and confirm that your CMakeCache.txt is the same? If so, this LGTM.
May 17 2019
May 16 2019
I believe MinGW lowercases all their headers, so that's the convention LLVM has been following, since cross-compilation using the Windows SDK generally involves some sort of case correction (through VFS overlays or CIOPFS or similar) anyway.
This is a straightforward build system fix, so it should be fine.
May 14 2019
Looks good, though I'm not super familiar with the libObject APIs (yet) :)
May 10 2019
May 8 2019
D61688 implements -z common-page-size.
May 7 2019
Will address any additional comments post-commit. Thanks for the quick review!
May 6 2019
May 3 2019
May 2 2019
LGTM, given it doesn't have any effect by default. I'm not sure if there are negative consequences around e.g. stack walking for crashes, but having the option shouldn't hurt.
Seems like you got the wrong diff.
Apr 30 2019
This is really nasty, but I'm not sure if there's a better way to do this.
Sorry, had missed this.
Last time this came up, we wanted to implement a check to ensure no source files were forgotten, as in do a glob and compare to the explicit list of files. The original plan was to use llvm_process_sources for that, but we couldn't do that cos libc++'s build can't rely on LLVM being around yet (though maybe we ought to reconsider that at some point with the upcoming monorepo move...). Do you think it's worth implementing that?
Apr 29 2019
LGTM with the indent fixed.
Apr 25 2019
This is awesome!
I feel something like llvm::sys::path::PathListSeparator would be nicer, since AFAIK any path list should be separated with semicolons on Windows and colons everywhere else.
LGTM, though you'll wanna wait for a libc++ code owner too.
LGTM, though I'd wait for Hans and/or Reid too.
Apr 24 2019
This would break if someone does a debug and release build simultaneously, the copies occur simultaneously and the filesystem gets unhappy, That seems fairly contrived though, and this is also restoring the behavior before your previous change.
Apr 23 2019
Apr 22 2019
@tstellar ping. Someone appears to be running into this on the CMake mailing list too: https://cmake.org/pipermail/cmake/2019-April/069359.html