Enable Monolithic build #9948
Enable Monolithic build #9948
Conversation
✅ Deploy Preview for meta-velox canceled.
|
307cbcf to
4b38c9d
Compare
|
I rebased, this should fix the test failures but the build works so I think this is review ready. @kgpai I didn't add the license header to the dbgn cmakelists as it's absence looked intentional? Should we add it or exclude the file from the test? |
5c50a2c to
d28f42f
Compare
| line_width: 80 | ||
| tab_size: 2 | ||
| use_tabchars: false | ||
| max_pargs_hwrap: 4 |
There was a problem hiding this comment.
The default for this is six which I think is just a bit too much as it get's hard to read.
|
While testing the new format I noticed that there are A LOT of files that are not correctly formatted but didn't show up because we only look at touched files. You can repro this by running We should probably apply formatting across everything, merge that as a standalone commit and than create a git blame revs ignore file to avoid that commit showing up everywhere when people git blame. @kgpai @pedroerp @mbasmanova ? |
72c885f to
5215794
Compare
301ef89 to
bc16b11
Compare
|
@assignUser |
|
@assignUser |
|
That's because of the changes to cake format now needing pyyaml. Once the container is rebuild it will pass. But I can also add temporary line to the job. |
Let's add a temporary line to ensure it passes. |
d4dbef4 to
0136b90
Compare
|
I have opened #10262 as a follow up. |
9581833 to
1bd5e1b
Compare
|
@assignUser can you rebase with main to get the linux CI jobs running? |
1bd5e1b to
f7ad166
Compare
|
@majetideepak done, thanks |
| @@ -68,6 +69,7 @@ option( | |||
| "Build a minimal set of components, including DWIO (file format readers/writers). | |||
| This will override other build options." | |||
| OFF) | |||
| option(VELOX_MONO_LIBRARY "Build single unified library." OFF) | |||
There was a problem hiding this comment.
nit: instead of MONO should we call it MONOLITHIC or UNIFIED (I prefer monolithic) . Unfortunately there are many projects called mono and saying MONOLITHIC just makes it very explicit.
| # Create a library for each invocation. | ||
| velox_base_add_library(${TARGET} ${library_type} ${ARGN}) | ||
| endif() | ||
| velox_install_library_headers() |
There was a problem hiding this comment.
Doesnt velox_base_add_library install library headers ?
|
@assignUser there is a new merge conflict. |
|
@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Hi @assignUser I'm seeing a conflict in pyvelox/CMakeLists.txt, do you mind rebasing? |
670f995 to
6298482
Compare
|
@kevinwilfong done :) |
|
@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kevinwilfong merged this pull request in 9f64e42. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
This PR adds an option that combines all targets into a single
veloxtarget and build a single unifiedlibvelox.a. This is entirely optional and defaults to off. This will make it easier for downstream users to use Velox by just linking tovelox::veloxin their own cmake instead of having to look through our cmake which targets they need. (Once we have a config etc.).I have yet to test the install functionality (it's strictly a bonus, the monolithic build is the goal for now).