pganalyze / pg_query.rs

Rust library to parse, deparse and normalize SQL queries using the PostgreSQL query parser
MIT License
126 stars 12 forks source link

use proper OUT_DIR for faster rebuilds #1

Closed NAlexPear closed 2 years ago

NAlexPear commented 2 years ago

According to the cargo book, any output from build scripts should be placed in the OUT_DIR directory. The current build script on main modifies assets in-place without respecting OUT_DIR, which might interfere with re-run logic from tools like cargo watch.

This PR brings back the OUT_DIR, but tries to minimize the number of files brought over on each rebuild. In practice (on my machine), this has eliminated some of the flakiness around cargo watch when working with the main branch, and has reduced fresh build times from ~30s to ~10s.

seanlinsley commented 2 years ago

@NAlexPear thanks for the PR!

@uhoh-itsmaciek could you give this PR a try locally? You were running into permissions issues before https://github.com/pganalyze/pg_query.rs/commit/5c4d1f2bd70e8b60c75994426651ca54048bf1d2 switched us to compile the C code in place.

msakrejda commented 2 years ago

Hmm. Initially cargo build ended in

   ...
   Compiling pg_query v0.7.0 (/home/maciek/code/pg_query.rs)
error: could not find native static library `pg_query`, perhaps an -L flag is missing?

error: could not compile `pg_query` due to previous error

So I tried to build main to see if I had the same problem there, but it built fine. So I tried the build on this branch again to confirm the problem, and now it builds fine as well (even after a cargo clean). I can't reproduce the error.

So I think this is fine, but any ideas on that error?

NAlexPear commented 2 years ago

@uhoh-itsmaciek hmm, I couldn't repro either, even with a full cargo clean and a purge of ~/.cargo/registry on two operating systems (macOS and Linux).

My first guess would be residual assets from in-place compilation on main before OUT_DIR was respected, but I'll admit to some recency bias there.

Would testing this hypothesis be a blocker for merge, or would you be satisfied with the current evidence that subsequent rebuilds aren't affected?

msakrejda commented 2 years ago

Thanks for checking. I think since I can't repro either, it's probably fine, but I'm not much of a Rust user so I'll defer to @seanlinsley.

seanlinsley commented 2 years ago

Hmm, I was able to reproduce the issue after purging libpg_query of build artifacts (which should be the default state for new builds).

cd libpg_query
rm -rf tmp
rm libpg_query.a
rm -rf test/*
git reset --hard

After doing that and then cargo clean, cargo build now consistently fails for me.

I think this line is the cause. Shouldn't it be linking to out_dir now?

println!("cargo:rustc-link-search=native={}", build_path.display());

Ah, yep, it builds successfully after making that change.

NAlexPear commented 2 years ago

Good catch @seanlinsley! I've updated the PR with the correct link search.

seanlinsley commented 2 years ago

Great, thanks again for the PR