Fix prepare race condition in workspaces

[cycraig]

Description of change

Small PR to change the target directory for offline query caches from target/sqlx to target/sqlx/<CRATE_NAME>, which fixes the behaviour seen in #2049. CC @djc

The problem is essentially a race condition: there is a period between cargo sqlx prepare deleting the target/sqlx directory and reading the queries from it, during which time another process could build/check one of the other crates in the workspace and populate it.

BEFORE, queries for all crates in a workspace would share the target/sqlx directory:

target/sqlx
├── query-2b192bf413d46a28226de2ad754a382043c483cf55c04d19bdfa916dcbf0e7ae.json
├── query-95d796755ea0ade4578c91d102407ae003487e40922284cdd6f5c8cbc8d0b5e2.json
└── query-e46f12d720e0911c9280a1b43d7aab30beef94861713fa686db77cfe30702792.json

AFTER this PR, queries will be placed under their respective crate names:

target/sqlx
├── crate-b
│   └── query-2b192bf413d46a28226de2ad754a382043c483cf55c04d19bdfa916dcbf0e7ae.json
├── crate_ç
│   └── query-4750859a2a2de4ee1e5a5c2cf07ba4ca66a31824e0f4504e21640a42081d95c0.json
└── cratea
    └── query-95d796755ea0ade4578c91d102407ae003487e40922284cdd6f5c8cbc8d0b5e2.json

You can validate the current and corrected behaviour in the following example workspace https://github.com/cycraig/sqlx_prepare_bug , which can (sometimes) reproduce the non-deterministic behaviour of pulling different queries into sqlx-data.json when multiple crates are being compiled during cargo sqlx prepare using sqlx 0.6.1 (it doesn't happen often for me, depends a lot on timing).

Note that crate names should be fine to create directories from, cargo does it all the time in the target directory. I'm not aware of any platforms where this would be an issue.

I believe resolve.root returned by cargo metadata should always be populated using any recent cargo version; the convenience .root_package() function I used has been around for two years at least but I did have to update the test fixture to include it (was null). Only the prepare command uses Metadata as far as I know, and it requires a non-virtual manifest, so it would error anyway if resolve is missing, e.g. when run in workspace roots. Edit: failed to account for cargo sqlx prepare --merged initially, updated PR.

Edit: I kept the behaviour of prepare nuking the entire target/sqlx directory just in case. Deleting only the relevant sub-crate directory would only be a minor optimisation and it's unclear to me what else it might affect. The entire directory should be deleted for cargo sqlx prepare --merged anyway.

Edit2: one alternative to this approach might be to pass an environment variable to the cargo check invocation in cargo sqlx prepare to use a different sub-directory during query caching, but I don't see any advantage to that.

Related Issue

Resolves #2049.

Type of change

Not sure if this qualifies as a breaking change, does anyone depend on the location of the query caches in target/sqlx? Otherwise it's just a bug fix.

How the change has been tested

cd sqlx-cli && cargo test and manually running the example in https://github.com/cycraig/sqlx_prepare_bug

[DXist]

There is a related problem with a single fixed location of sql-data.json per crate. A crate could have multiple targets.

Let's imagine a use case when the lib target may or may not use offline mode (depends on the preference of particular developer).

But there are also multiple targets that contain integration tests with own test-only sql queries. If a developer prefers offline mode than an IDE with rust-analyzer shows lint warnings for queries in integration test modules because the generated for the lib target sql-data.json file doesn't have test-specific queries.

[cycraig]

Edit: this comment is irrelevant now, see the workaround in #2069 (comment) .

Is that issue only with test-gated queries or are there problems with other cases too? I imagine for bin versus lib one would just prepare for both at once. Edit: apparently that's not possible right now :P

We could separate the files for test targets based on cfg(test) in query, and pass another flag to prepare maybe.

E.g. output sql-data.json and sql-data-test.json separately, and query caches would be output to target/sqlx/<CRATE_NAME> or target/sqlx/<CRATE_NAME>-test accordingly. The sql-data-test.json and cache directories would contain duplicate query definitions from the regular targets, but I don't think that's a major problem.

Is there a more elegant solution by appending a test section to sql-data.json or maybe per target? That would be a larger breaking change and easy to introduce bugs with stale data or bad overwrites though.

Maybe a simpler solution for that problem would be for prepare always to pass --cfg test to rustc? (Cannot currently be passed via cargo_args). Edit: tried this and it doesn't actually work as easily as that, but might be another direction to explore to enable multiple targets for prepare in the same sql-data.json.

It might be better to have a followup PR or issue either way, since it depends on the approach?

[djc]

Since prepare is run on a per-target basis, it definitely seems possible to have, say, prepare -- --lib and prepare -- --bin main get into a race condition in the same way as separate crates might have. So I guess the straightforward fix here is to also have a target directory within the crate directory inside target/sqlx?

I've definitely felt the pain of not being able to have multiple targets in sqlx-data.json at the same time (for us, mostly examples and lib), so maybe that's something we can explore after this is merged.

[abonander]

The better approach may be to revive #1770. It's still something we want, it just didn't make it into 0.6.0.

[DXist]

@cycraig , regarding cfg(test) - this only works for unit tests inside a target. Integration tests are different targets and can't use cfg(test) intended for the lib target.

[djc]

The better approach may be to revive #1770. It's still something we want, it just didn't make it into 0.6.0.

Are you suggesting you don't want to review/merge this, or that you don't want to make this more complex? How much work do you think it will take to finish #1770?

[cycraig]

regarding cfg(test) - this only works for unit tests inside a target. Integration tests are different targets and can't use cfg(test) intended for the lib target.

@DXist I think generating sqlx-data.json for multiple targets can already be achieved by passing them to cargo through sqlx prepare? But only with the --merged flag though (works for the current crate in a subdirectory just fine too). E.g. cargo sqlx prepare --merged -- --tests --examples --lib --bins etc. Without --merged it calls cargo rustc which cannot handle multiple targets. Both should probably go through cargo check but at least there's a workaround without any changes needed?

Edit: opened #2071 so multiple targets can work with or without --merged.

[DXist]

@cycraig, indeed. I added --merged and --tests and now the file contains all queries used during the development.

Thank you!

[abonander]

This PR will be obviated by #1770 but I overestimated from the discussions how complex this solution was so I'm happy to accept it for the time being.