Page MenuHomePhabricator

Make framework-header-fix process copied headers
ClosedPublic

Authored by keith on Jul 25 2018, 12:43 AM.

Details

Summary

Previously the framework-header-fix script would change the sources
before they were copied, leading to unnecessary rebuilds on repeat
ninja lldb invocations. This runs the script on the headers after
they're copied into the produced LLDB.framework, meaning it doesn't
affect any files being built.

Diff Detail

Repository
rL LLVM

Event Timeline

keith created this revision.Jul 25 2018, 12:43 AM

It seems like if this was a common occurrence, it would've been fixed earlier, so I'm wondering if there's a difference in the way I'm building lldb that causes this. Using cmake:

cmake ../llvm -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=YES -DLLDB_EXPORT_ALL_SYMBOLS=1 -DLLDB_BUILD_FRAMEWORK=ON

And then ninja lldb, repeat ninja lldb builds causes ~500 files to be rebuilt with no source changes. I've diffed the LLDB.framework before and after and there don't seem to be any differences in the produced headers.

labath added a subscriber: labath.

Adding Alex, as he was doing a lot of framework work lately.

If I understand correctly, this is putting the headers directly into the framework? That's a pretty good idea :D

It seems like if this was a common occurrence, it would've been fixed earlier, so I'm wondering if there's a difference in the way I'm building lldb that causes this. Using cmake:

cmake ../llvm -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=YES -DLLDB_EXPORT_ALL_SYMBOLS=1 -DLLDB_BUILD_FRAMEWORK=ON

And then ninja lldb, repeat ninja lldb builds causes ~500 files to be rebuilt with no source changes. I've diffed the LLDB.framework before and after and there don't seem to be any differences in the produced headers.

That's roughly how I build LLDB as well, give or take a few options.

cmake/modules/LLDBFramework.cmake
21 ↗(On Diff #157201)

This line shouldn't be necessary anymore then, right?

keith added inline comments.Jul 25 2018, 10:18 AM
cmake/modules/LLDBFramework.cmake
21 ↗(On Diff #157201)

It still is, the change below doesn't actually do the copying, it only runs the script on the location the headers have already been copied too.

xiaobai requested changes to this revision.Jul 26 2018, 10:11 AM

I think this might actually not do what we want it to do. I've commented inline with my concerns.

cmake/modules/LLDBFramework.cmake
21 ↗(On Diff #157201)

Are you saying that it will fix the framework headers that have already been copied? If so, then I don't think this will work, because the copy occurs after lldb-framework has been built, and lldb-framework depends on lldb-framework-headers.

41 ↗(On Diff #157201)

This should not just depend on framework_headers, because those are in the location ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders. You want to run the script on $<TARGET_FILE_DIR:liblldb>/Headers, which is not guaranteed to exist or have been populated with headers when this actually gets run. See my comment above.

This revision now requires changes to proceed.Jul 26 2018, 10:11 AM
keith updated this revision to Diff 157525.Jul 26 2018, 10:24 AM
  • Make headers a post build command
keith added inline comments.Jul 26 2018, 10:24 AM
cmake/modules/LLDBFramework.cmake
41 ↗(On Diff #157201)

Updated this by making it a POST_BUILD command instead

Looks good

cmake/modules/LLDBFramework.cmake
41 ↗(On Diff #157201)

Can you combine this with the lldb-framework POST_BUILD above? After that this should be good to go.

keith added inline comments.Jul 26 2018, 11:06 AM
cmake/modules/LLDBFramework.cmake
41 ↗(On Diff #157201)

If I combine it with the one above I'd have to do it twice I think?

xiaobai added inline comments.Jul 26 2018, 11:18 AM
cmake/modules/LLDBFramework.cmake
41 ↗(On Diff #157201)

I think if you insert the command after moving the headers then you shouldn't have to do it twice

keith added inline comments.Jul 26 2018, 11:25 AM
cmake/modules/LLDBFramework.cmake
41 ↗(On Diff #157201)

I might be misunderstanding the suggestion. I think I would have to after line 21 and line 28

xiaobai added inline comments.Jul 26 2018, 11:27 AM
cmake/modules/LLDBFramework.cmake
41 ↗(On Diff #157201)

Oh, completely forgot about the line 28 case. In that case, why not pull out the copying of headers to right above this one then? Should allow us to prevent duplicating that logic.

keith updated this revision to Diff 157547.Jul 26 2018, 11:54 AM
  • Hoist LLDB.framework headers copy out of condition
keith added inline comments.Jul 26 2018, 11:55 AM
cmake/modules/LLDBFramework.cmake
41 ↗(On Diff #157201)

Done!

xiaobai accepted this revision.Jul 26 2018, 11:58 AM

Awesome! Thank you so much for your patience and contribution. :)

Do you need somebody to commit this for you?

This revision is now accepted and ready to land.Jul 26 2018, 11:58 AM

No problem! Yes please! :)

This revision was automatically updated to reflect the committed changes.