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

Try avoiding a full clean in cargo sqlx prepare --merged #1802

Merged
merged 7 commits into from
Jul 12, 2022

Conversation

CosmicHorrorDev
Copy link
Contributor

@CosmicHorrorDev CosmicHorrorDev commented Apr 14, 2022

Closes #1793

This PR avoids a full cargo clean when running cargo sqlx prepare --merged by selectively cargo clean -ping packages that depend on sqlx-macros outside the current workspace and updating the mtime of the source paths of crates that depend on sqlx-macros within the current workspace.

Doing recompiles this way drastically lowers the time that cargo sqlx prepare --merged takes. I'll avoid a benchmark this time since it's just comparing a full recompile with a partial recompile and depends heavily on the project it's being run on and the machine doing the building, but it's a drastic improvement.


Update: Even though cargo clean -p <SPEC> works by pkgid, it ignores the url and version to just clean by name. Due to that I ripped out all the pkgid logic

Currently the changes don't allow for people to ignore crates that depend on sqlx-macros without actually using compile-time verified queries. My original idea on this was to allow people to pass pkgids of crates they want ignored. From there the fully qualified pkgid spec could be obtained from running cargo pkgid <partially-qualified-spec>, and those fully qualified specs can be removed from the fully qualified specs returned from cargo metadata. I'll just paste in a md formatted version of my comment in the code:

NOTE: Trying to get the url from source is rather problematic:

  • Local files don't have this set
    • It only appears to be set within id which would need to be parsed
  • ssh urls (from using git repos) have can include fragments and queries that pkgid does not expect

A separate issue is that pkgids don't seem to be entirely unique (two git urls on different commits with the same version and name get normalized to the same pkgid)

Due to all of this we:

  • Only allow crates.io as the source url
  • Can't use PkgId as a UUID, instead we need to use the metadata id still
  • May recompile more packages then required due to ambiguity

I'm planning on opening issues upstream about both of the above problems upstream, but the good news is that adding an --ignore flag shouldn't be a breaking change since its behavior is opt-in

@CosmicHorrorDev
Copy link
Contributor Author

Got a few more changes I want to make on this later today. I'll comment after I get everything pushed

@CosmicHorrorDev
Copy link
Contributor Author

CosmicHorrorDev commented Apr 15, 2022

Final set of changes are up! Things changed a bit since my original description, so here's the current gist:

This PR manages to avoid a full clean when using cargo sqlx prepare --merged by inspecting the dependency graph to find all crates that depend on sqlx-macros which will need to be recompiled to ensure that any changes with the compile-time verified queries are detected. Crates within the workspace that need recompiling will have their source path's mtime updated, while crates outside the workspace will be selectively cargo clean -ped by crate name

#1793 also mentioned allowing for an ignore list so that crates that depend on sqlx-macros, but don't use compile-time verified queries can be ignored when setting up a recompile. This wasn't implemented in this PR because:

  1. This is the more "bullet-proof" portion of the logic for setting up recompiles, and helps lay the foundation for later changes
  2. Ignoring crates would be opt-in so we should be able to add that functionality later without having to wait for a breaking-change release

Feel free to let me know what you think of the changes. I verified that the changes appear to work fine on a few projects I have that use sqlx.

It's also worth noting that this currently cleans sqlx (because it depends on sqlx-macros) which ends up causing all the sqlx* crates to get recompiled due to rust-lang/cargo#10069. My plan was to have sqlx be ignored so that this wouldn't occur, but I'm trying to keep the scope of this PR limited

@CosmicHorrorDev
Copy link
Contributor Author

Also wanted to say it's a bit unfortunate that github is counting the pretty-printed json file when it comes to the +/- for code. I can compact it, or change the test. There's just a lot of complexity contained within figuring out the recompile action, so I wanted a test to make the behavior obvious

@abonander
Copy link
Collaborator

@LovecraftianHorror sorry for the delay, you mind rebasing and fixing the merge conflicts?

@CosmicHorrorDev
Copy link
Contributor Author

I'll try to get to that later today

@CosmicHorrorDev
Copy link
Contributor Author

Merge conflicts fixed, and no worries on the delay. I understand that life gets busy

I don't know if I mentioned elsewhere in this thread, but these changes should be backwards compatible which means that it shouldn't have to wait for another breaking version update

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 this pull request may close these issues.

Overhauling cargo sqlx prepare --merged
2 participants