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

Use cargo check consistently in prepare #2071

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Aug 25, 2022

Description of change

Small PR to change cargo sqlx prepare to always use cargo check.

Currently, prepare invokes cargo rustc by default but switches to check when the --merged flag is used. This can lead to confusion since certain flags passed to cargo check will be rejected by cargo rustc when running prepare.

The current behaviour also contradicts the sqlx-cli README, which states:

In order for sqlx to be able to find queries behind certain feature flags you need to turn them on by passing arguments to rustc.

This is how you would turn all targets and features on.

cargo sqlx prepare -- --all-targets --all-features

Which actually fails with the following error:

error: extra arguments to `rustc` can only be passed to one target, consider filtering
the package by passing, e.g., `--lib` or `--bin NAME` to specify a single target
error: `cargo check` failed with status: exit code: 101   

But passes with --merged, regardless of whether the current directory is a workspace root or a sub-crate.
cargo sqlx prepare --merged -- --all-targets --all-features

This PR ensures it works with or without --merged.

When the working directory is a crate in a workspace, it still only builds the queries for it and not the entire workspace.
Is there any reason to retain the behaviour of using rustc directly? Edit: does this negatively affect plans for #1706?

This arose from the following comment thread: #2069 (comment)

Alternative: update the documentation/README.md to specify --merged must be used to combine the queries of multiple targets, even when in a single crate.

P.S. sorry for spamming PRs.

Type of change

Bug fix to match the intended behaviour, but might technically be a breaking change as it changes which flags are accepted or not.

How the change has been tested

cd sqlx-cli && cargo test and running cargo sqlx prepare -- --all-targets --all-features in a sample project.

@abonander
Copy link
Collaborator

This PR has conflicts after merging #2069.

@cycraig cycraig force-pushed the chore/use-cargo-check-prepare branch from eb9d3f4 to b331f3f Compare August 27, 2022 08:18
@cycraig
Copy link
Contributor Author

cycraig commented Aug 27, 2022

Thanks for the ping; rebased the changes.

@abonander abonander merged commit 20af5cd into launchbadge:main Sep 1, 2022
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.

2 participants