Skip to content

Conversation

@CathalMullan
Copy link

This ties the offline feature to the macros feature, allowing for builds without serde.

I opted to make the Executor::describe function feature gated on offline. This seemed like the simplest option, assuming it only exists to serve the macro feature. But I'd understand if you'd wanted to go with a different approach.

Outside of the feature cfg additions, I also had to remove the use of serde_urlencoded from sqlx-sqlite, manually encoding the key/values instead.

One issue is that the CI currently doesn't test building without the macros feature at all.

In terms of compile time performance, running LIBSQLITE3_SYS_USE_PKG_CONFIG=1 cargo build --release --no-default-features --features runtime-smol,migrate,all-databases resulted in a drop from 28s to 21s with these changes.

Does your PR solve an issue?

Fixes #4070

Is this a breaking change?

Yes, due to altering features at the sqlx level.
Though this will only impact anyone setting default-features = false.

And due to changing the executor sqlx-core API by adding the feature gate.

}

/// Check whether EXPLAIN statements are supported by the current connection
#[cfg(feature = "offline")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given how much needs to be #[cfg]'d here, it's probably best to split the EXPLAIN parsing into a sibling or child module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid making the naming too confusing (since describe itself is what's being conditionally compiled), maybe all the non-conditional stuff should be moved to a new module, maybe named resolve?

The easiest order of operations might be to:

  • Move the #[cfg]'d stuff to a module named describe_tmp or whatever
  • Rename this module to resolve
  • Rename describe_tmp to describe

Copy link
Author

Choose a reason for hiding this comment

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

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.

Ability to use sqlx without serde (and potentially syn)

2 participants