Skip to content

Support workspaces for offline feature #506

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

Merged
merged 1 commit into from
Dec 19, 2020

Conversation

jgrund
Copy link
Contributor

@jgrund jgrund commented Jul 10, 2020

This patch enables having a top-level sqlx-data.json file within a
workspace.

It does this by using a full clean / check instead of cargo rustc
which fails on a workspace.

It also uses the cargo_metadata package to find the workspace_root
(which should be the same as CARGO_MANIFEST_DIR in a non workspace).

Note: cargo_metadata pulls in serde / serde_json. I wasn't sure if I
should remove the features for those with this change.

Fixes #353.

Signed-off-by: Joe Grund [email protected]

@mehcode mehcode requested a review from abonander July 12, 2020 10:54
Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, also while I don't disagree with the goal of this PR I'm not sure everyone using cargo sqlx prepare will want a single merged sqlx-data.json at the workspace root so I think there's room for a more flexible solution. Maybe we should put this behavior under a --merged flag?

@@ -40,7 +53,7 @@ pub fn expand_input(input: QueryMacroInput) -> crate::Result<TokenStream> {

#[cfg(feature = "offline")]
None => {
let data_file_path = std::path::Path::new(&manifest_dir).join("sqlx-data.json");
let data_file_path = CRATE_ROOT.join("sqlx-data.json");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get the behavior you want, all you actually have to do is save sqlx-data.json to the current working directory (so not even joining it with any path); that will always be the workspace root in the case of proc macros.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think we maybe want to distinguish between cases where the user wants a single merged sqlx-data.json file at the workspace root and where the user wants an individual sqlx-data.json file per crate.

@mehcode any opinions?

Copy link
Contributor Author

@jgrund jgrund Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing CARGO_MANIFEST_DIR is the crate root, not the workspace root.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannik4
Copy link
Contributor

jannik4 commented Jul 13, 2020

+1 for something like a --merged flag. I have multiple database crates in a workspace where a merged sqlx-data.json would be problematic. (I think something like a --all flag where the command generates a sqlx-data.json for each crate would be useful, but running the command for each crate separately is not too bad either.)

@jgrund
Copy link
Contributor Author

jgrund commented Jul 14, 2020

I'll update to use a --merged flag.

@abonander
Copy link
Collaborator

We prefer not to merge master back into feature branches to update them as it muddies the history; we always prefer to rebase instead.

@jgrund
Copy link
Contributor Author

jgrund commented Jul 20, 2020

I'll rebase once I have updates to push, for now I'll move into draft

@jgrund jgrund marked this pull request as draft July 20, 2020 14:46
@jgrund jgrund force-pushed the support-offline-workspaces branch from cb019ca to 7ed185a Compare July 20, 2020 15:00
@mehcode
Copy link
Member

mehcode commented Nov 12, 2020

We can merge this once its updated to use a --merged flag. I'm not super keen on the name but I don't want to block this on bikeshedding it.

This patch enables having a top-level `sqlx-data.json` file within a
workspace.

It does this by using a full clean / check instead of `cargo rustc`
which fails on a workspace.

A `--merged` flag is introduced to switch to the workspace behavior

Fixes launchbadge#353.

Signed-off-by: Joe Grund <[email protected]>
@jgrund jgrund force-pushed the support-offline-workspaces branch from 3aca4f9 to 83d3f8c Compare December 9, 2020 15:46
@jgrund jgrund marked this pull request as ready for review December 9, 2020 15:57
@jgrund jgrund requested a review from abonander December 9, 2020 15:57
@mehcode
Copy link
Member

mehcode commented Dec 19, 2020

This looks good to me. Thank you @jgrund for hanging in there.

@mehcode mehcode merged commit 3e1da43 into launchbadge:master Dec 19, 2020
@jgrund
Copy link
Contributor Author

jgrund commented Dec 21, 2020

@mehcode Thanks, can you release a new version of sqlx-cli as well?

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

Successfully merging this pull request may close these issues.

[sqlx-cli] potential issue in workspace crates
4 participants