-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
fix(sqlx-cli): do not clean sqlx during prepare #2786
base: main
Are you sure you want to change the base?
Conversation
The unit test is failing because it checks that the Edit: not sure how to update the test right now without losing coverage or adding +10,000 useless lines of Cargo.lock, may come back to this later. |
The only reason we clean anything in the first place is it seems like the only surefire way to force a recompile so that the proc macros actually execute. If you have any thoughts there, we could get rid of the cleaning entirely. |
Unfortunately it looks like Regarding avoiding cleaning dependencies unnecessarily, I'm still encountering problems with this patch due to feature unification. E.g. pgvector-rust disables all default features of
sqlx-macros v0.7.2 (proc-macro)
└── sqlx v0.7.2
├── example v0.1.0 (/tmp/example)
└── pgvector v0.3.2
└── example v0.1.0 (/tmp/example) This is just expected behaviour; I think we would need a way to isolate the features/dependencies of a particular dependency to avoid it (assuming the clean behaviour is kept)? Maybe a CLI argument could be added as a manual workaround, e.g. |
Would the fix maybe be just cleaning workspace crates? Or just the current crate? |
There may be out-of-workspace dependencies that contain query macros, so I don't think that would work in general.
|
Ignoring non-workspace crates seems fine to me. Those should be prepared separately. |
Is that possible, to vendor prepared query macros from dependencies? How can query macros from a
be prepared to ensure they're valid against a different database? I'm all for ignoring (not cleaning) non-workspace crates, but there do seem to be valid, if uncommon, cases where it may be needed. Maybe just hide the behaviour behind a coarse CLI flag? (As opposed to a fine-grained flag suggested in my previous comment). E.g.
In both cases, |
Description
Fix
cargo sqlx prepare --workspace
runningcargo clean -p sqlx
unnecessarily.See #2785
This should be a non-breaking change. It shouldn't affect in-workspace dependencies that happen to be named
sqlx
either.How to test or review this PR
sqlx-cli
.cargo sqlx prepare --workspace
in a project with crates that usequery!
macros.The timing difference is quite significant in a larger workspace, saves ~40 seconds on an incremental build.
BEFORE:
AFTER (this PR, note there is no cleaning step):