Skip to content
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

Overhauling cargo sqlx prepare --merged #1793

Closed
CosmicHorrorDev opened this issue Apr 11, 2022 · 4 comments · Fixed by #1802
Closed

Overhauling cargo sqlx prepare --merged #1793

CosmicHorrorDev opened this issue Apr 11, 2022 · 4 comments · Fixed by #1802

Comments

@CosmicHorrorDev
Copy link
Contributor

I wanted to get some feedback before submitting my idea as a PR.

Currently cargo sqlx prepare --merged runs a cargo clean to ensure a recompile on all the compile-time queries which is rather heavy-handed. I'll enumerate the attempts and other information I've come across along the way, and then follow it up with the current technique I was thinking of

Removing cargo clean?

Without --merged prepare uses cargo rustc to compile the crate with a ever-changing --cfg to trigger a consistent recompile. --merged uses cargo check instead because cargo rustc can't be run on a workspace. If we remove the clean and just try cargo check with an ever changing --cfg then it still triggers a full recompile with CARGO_LOG=cargo::core::compiler::fingerprint=info indicating that it's due to the differing --cfg

[2022-04-11T03:03:51Z INFO  cargo::core::compiler::fingerprint]     err: RUSTFLAGS has changed: previously ["--cfg", "__sqlx_recompile_trigger=\"foo\""], now ["--cfg", "__sqlx_recompile_trigger=\"bar\""]

Removing the ever-changing --cfg too runs into the issue of queries not actually being compiled and getting missed unsurisingly

Running multiple cargo rustcs?

We could invoke multiple cargo rustc commands to cover all the crates we need. The issues here are

  1. Juggling all the feature information to make sure we're compiling crates with the additive features that would have when compiled together
  2. Hard to ensure that we can emulate how flags that would normally be passed to cargo check would be passed to individual cargo rustcs like --features

Triggering recompiles without a full clean

The implementation that's stuck with me so far is to extend Removing cargo clean where cargo clean and the ever-changing --cfg is removed. Instead recompiles are triggered by modifying the mtime of crates within the workspace and by cargo clean --package [<SPEC>]ing crates outside of the workspace. The list of crates to touch/clean are all crates in the dep graph that depend on sqlx-macros somewhere in their dep tree with an ignore list that can be specified to avoid recompiling crates that are known to depend on sqlx-macros without actually containing any compile-time queries themselves (e.g. sqlx and ormx)

New CLI structure

(Feel free to bikeshed naming and all if we're going to end up changing stuff anyways)

We need some way to be able to pass the ignorelist mentioned above. The easiest way I could think of was to switch --merged to a subcommand that has an --ignore flag that can be used to specify crates

$ cargo sqlx prepare merged --ignore uses_sqlx_macros_without_queries --ignore another_crate_to_ignore
@abonander
Copy link
Collaborator

So how does this interact with #1770 which I want to release this week as 0.6.0?

@CosmicHorrorDev
Copy link
Contributor Author

From a quick peek it looks like #1770 still triggers a recompilation with a cargo clean followed by cargo check for workspaces, so nothing should be different with the internals

For the CLI bits it looks like the same changes would be needed, but with s/merged/workspace since the flag name was changed

I didn't realize that 0.6.0 was planned so soon. I could get the changes that I mentioned out on Wednesday and fix any merge conflicts with #1770 which should be minor. I also understand if you don't want to hold up the release on these changes and would want to slot it for 0.7.0 instead

@abonander
Copy link
Collaborator

Yeah I have a number of breaking dependency upgrades (like time) that have been piling up. I was going to release 0.5.12 as the backwards compatible subset and then immediately release 0.6.0

@CosmicHorrorDev
Copy link
Contributor Author

Just for reference, I have an implementation hashed out for this. Just need to tidy things up and then I'll throw a PR up for it later today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants