Enable Monolithic build #9948
Enable Monolithic build #9948
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
assignUser
force-pushed
the
monolithic-build
branch
2 times, most recently
from
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? |
assignUser
force-pushed
the
monolithic-build
branch
2 times, most recently
from
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 ? |
assignUser
force-pushed
the
monolithic-build
branch
from
72c885f
to
5215794
Compare
assignUser
force-pushed
the
monolithic-build
branch
2 times, most recently
from
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. |
assignUser
force-pushed
the
monolithic-build
branch
from
d4dbef4
to
0136b90
Compare
|
I have opened #10262 as a follow up. |
assignUser
force-pushed
the
monolithic-build
branch
from
9581833
to
1bd5e1b
Compare
|
@assignUser can you rebase with main to get the linux CI jobs running? |
assignUser
force-pushed
the
monolithic-build
branch
from
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 ?
kgpai
added
the
ready-to-merge
|
@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? |
assignUser
force-pushed
the
monolithic-build
branch
from
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).