Skip to content

don't redownload duckdb for every branch #7747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
myrrc wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

don't redownload duckdb for every branch #7747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,8 @@ use_debug = "deny"

[profile.release]
codegen-units = 1
# Turn LTO off, as it breaks when vortex-duckdb-ext is linked.
# Our general performance should not be dependent on LTO since crates that use
# Vortex may choose not to have LTO.
lto = "off"

[profile.release_debug]
Expand Down
24 changes: 14 additions & 10 deletions vortex-duckdb/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,10 +328,14 @@ fn c2rust(crate_dir: &Path, duckdb_include_dir: &Path) {
}
}

fn cpp(duckdb_include_dir: &Path) {
fn cpp(duckdb_include_dir: &Path, build_type: &str) {
let mut flags = vec!["-Wall", "-Wextra", "-Wpedantic", "-Werror"];
if build_type == "release" {
flags.extend_from_slice(&["-flto=auto"]);
Copy link
Copy Markdown
Contributor

@0ax1 0ax1 May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split out the LTO changes from this PR? Does enabling LTO for the C++ code in vortex-duckdb even have an effect on perf? Other than that, my idea was that we enable LTO in the ext repo but not in the vortex repo itself.

Copy link
Copy Markdown
Contributor

@0ax1 0ax1 May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto means full lto for clang which is very slow. we should try to use thin for both gcc and clang.

}
cc::Build::new()
.std("c++20")
.flags(["-Wall", "-Wextra", "-Wpedantic"])
.flags(flags)
.cpp(true)
.include(duckdb_include_dir)
.include("cpp/include")
Expand Down Expand Up @@ -392,8 +396,12 @@ fn main() {

let crate_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap());
let duckdb_dir = crate_dir.join("duckdb");
let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap());
let library_dir = out_dir.join(format!("duckdb-lib-{version}"));
// Cargo has changed OUT_DIR behaviour so for every branch and build we have
// a different directory e.g.
// ~/vortex/target/release/build/vortex-duckdb-b6bc1fb73230910e/out/duckdb-lib-v1.5.2
// We don't want this since artifacts are the same.
// We can't use CARGO_TARGET_DIR since it's out of tree.
let library_dir = duckdb_dir.join(format!("duckdb-lib-{version}"));

let library_dir_str = library_dir.display();
println!("cargo:rustc-link-search=native={library_dir_str}");
Expand All @@ -414,7 +422,7 @@ fn main() {
// Alternatively, set LD_LIBRARY_PATH (Linux) or DYLD_LIBRARY_PATH (macOS) at runtime.
println!("cargo:lib_dir={library_dir_str}");

let source_dir = out_dir.join(format!("duckdb-source-{version}"));
let source_dir = duckdb_dir.join(format!("duckdb-source-{version}"));
let source_archive_url = match &version {
DuckDBVersion::Release(v) => format!("{DUCKDB_SOURCE_RELEASE_URL}/v{v}.zip"),
DuckDBVersion::Commit(c) => format!("{DUCKDB_SOURCE_COMMIT_URL}/{c}.zip"),
Expand All @@ -429,10 +437,6 @@ fn main() {
extract(&source_archive_path, &source_dir);
}

drop(fs::remove_file(&duckdb_dir));
drop(fs::remove_dir_all(&duckdb_dir));
std::os::unix::fs::symlink(&source_dir, &duckdb_dir).unwrap();

let has_debug_env =
env::var("VX_DUCKDB_DEBUG").is_ok_and(|v| matches!(v.as_str(), "1" | "true"));
let build_type = match has_debug_env {
Expand All @@ -449,6 +453,6 @@ fn main() {

let duckdb_include_dir = inner_dir.join("src").join("include");
c2rust(&crate_dir, &duckdb_include_dir);
cpp(&duckdb_include_dir);
cpp(&duckdb_include_dir, build_type);
rust2c(&crate_dir);
}
13 changes: 6 additions & 7 deletions vortex-duckdb/cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@

# This CMake project is only used for IDE support (CLion, etc.).
# See build.rs for the actual build definition.
#
# Prerequisites: Run `cargo build -p vortex-duckdb` first to download DuckDB
# source and create the symlink at ../duckdb.
# source
#
# If your editor relies on a compilation database to enable LSP functionality,
# you can generate it by running `cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON` or
Expand All @@ -23,19 +22,19 @@ if (NOT CMAKE_BUILD_TYPE)
endif()

# Enable compiler warnings (matching build.rs flags).
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic -Werror")
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -Wall -Wextra -Wpedantic -Werror -O3")
Copy link
Copy Markdown
Contributor

@0ax1 0ax1 May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the release flags in this PR? Also should release just expand on default flags? -O0 will be overridden by -O3 etc.


# Find DuckDB include directory via the symlink created by build.rs.
# The symlink points to target/duckdb-source-vX.Y.Z which contains duckdb-X.Y.Z/
file(GLOB DUCKDB_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/../duckdb/duckdb-*")
# Find DuckDB include directory containing duckdb-source-vX.Y.Z which contains duckdb-X.Y.Z/
file(GLOB DUCKDB_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/../duckdb/duckdb-source-*/duckdb-*/")
if(DUCKDB_DIRS)
list(GET DUCKDB_DIRS 0 DUCKDB_DIR)
set(DUCKDB_INCLUDE "${DUCKDB_DIR}/src/include")
message(STATUS "Found DuckDB include: ${DUCKDB_INCLUDE}")
else()
message(FATAL_ERROR
"DuckDB source not found at ../duckdb/duckdb-*\n"
"Run 'cargo build -p vortex-duckdb' first to download DuckDB and create the symlink."
"Run 'cargo build -p vortex-duckdb' first to download DuckDB"
)
endif()

Expand Down
Loading