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

chore(madsim,deps): use patched sqlx managed by the madsim-rs organization instead of a personal repository #19130

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

kwannoel
Copy link
Contributor

@kwannoel kwannoel commented Oct 25, 2024

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

We use patched sqlx from the madsim fork, instead of the one in my personal fork of sqlx.

Please review the changes in that fork prior to reviewing this PR.

This accomplishes two things:

  1. Let the Madsim Org manage the fork. We can have PR reviews each time we rebase the patch.
  2. Inside the fork will adopt a rebase workflow, and continually patch for madsim on top of the tagged commit corresponding to the latest sqlx version.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

@kwannoel kwannoel requested a review from a team as a code owner October 25, 2024 08:47
@kwannoel kwannoel changed the title chore(deps): use sqlx managed by the risingwavelabs org instead of a personal repository chore(deps): use sqlx managed by the risingwavelabs organization instead of a personal repository Oct 25, 2024
@kwannoel kwannoel changed the title chore(deps): use sqlx managed by the risingwavelabs organization instead of a personal repository chore(madsim,deps): use patched sqlx managed by the risingwavelabs organization instead of a personal repository Oct 25, 2024
@kwannoel kwannoel force-pushed the kwannoel/use-rebased-sqlx branch from 21b8fa5 to 5d46528 Compare October 25, 2024 09:18
@kwannoel kwannoel enabled auto-merge October 27, 2024 15:46
@kwannoel kwannoel disabled auto-merge October 27, 2024 15:46
@kwannoel kwannoel enabled auto-merge October 28, 2024 01:29
Cargo.lock Outdated
@@ -9747,7 +9747,7 @@ dependencies = [
"indoc",
"libc",
"memoffset",
"parking_lot 0.12.1",
"parking_lot 0.11.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the downgrade here expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted it. Seems really strange. Each time I run cargo update sqlx, it keeps updating this dependency as well, for some reason.

@kwannoel kwannoel added this pull request to the merge queue Oct 28, 2024
@kwannoel kwannoel removed this pull request from the merge queue due to a manual request Oct 28, 2024
@BugenZhao
Copy link
Member

BugenZhao commented Oct 28, 2024

We use patched sqlx from the risingwave fork: risingwavelabs/sqlx#1, instead of the one in my personal fork of sqlx.

As the changes are solely to make sqlx compatible with madsim, I slightly prefer keeping it maintained under madsim-rs, especially when considering that we all have write permissions to madsim-rs now.

Otherwise, should we also move other forks, like tonic, rdkafka, tokio-postgres or even tokio-stream into risingwavelabs org?

@kwannoel
Copy link
Contributor Author

We use patched sqlx from the risingwave fork: risingwavelabs/sqlx#1, instead of the one in my personal fork of sqlx.

As the changes are solely to make sqlx compatible with madsim, I slightly prefer keeping it maintained under madsim-rs, especially when considering that we all have write permissions to madsim-rs now.

Otherwise, should we also move other forks, like tonic, rdkafka, tokio-postgres or even tokio-stream into risingwavelabs org?

Done. madsim-rs/sqlx#3.

@kwannoel kwannoel force-pushed the kwannoel/use-rebased-sqlx branch from 7bb1fe4 to 11024a5 Compare October 28, 2024 06:12
@kwannoel kwannoel changed the title chore(madsim,deps): use patched sqlx managed by the risingwavelabs organization instead of a personal repository chore(madsim,deps): use patched sqlx managed by the madsim-rs organization instead of a personal repository Oct 28, 2024
@kwannoel kwannoel added this pull request to the merge queue Oct 28, 2024
Merged via the queue into main with commit aea3886 Oct 28, 2024
37 of 38 checks passed
@kwannoel kwannoel deleted the kwannoel/use-rebased-sqlx branch October 28, 2024 08:00
BugenZhao pushed a commit that referenced this pull request Oct 28, 2024

# sqlx version: v0.7.4
# patch diffs: https://github.com/madsim-rs/sqlx/pull/3
sqlx = { git = "https://github.com/risingwavelabs/sqlx.git", rev = "ff6d6d2dc0e9e8e47282fd29be006eed7ae3421a" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed it to madsim, but didn't update it here 🤦 . Regardless they are using the same commit.

I will update this later when bumping sqlx to 0.8.2: #19145

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.

5 participants