Skip to content

Conversation

@prathyushpv
Copy link
Contributor

@prathyushpv prathyushpv commented Feb 13, 2025

What changed?

Use version v1.34.1 of modernc.org/sqlite library.

Why?

We had fixed an issue with contexts being cancelled during a transaction. But a later change in this library reintroduced this issue.
While we work with the maintainer to fix this, use the previous version.

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

No

@prathyushpv prathyushpv requested a review from a team as a code owner February 13, 2025 00:32
Makefile Outdated
endif

PINNED_DEPENDENCIES := \
modernc.org/sqlite@v1.34.0
Copy link
Member

Choose a reason for hiding this comment

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

Please document why this is pinned.

Copy link
Member

Choose a reason for hiding this comment

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

Can we also pin to a more recent patch or does it have to be 1.34.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use v1.34.1. v1.34.2 has the problematic commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment.

@rodrigozhou
Copy link
Contributor

rodrigozhou commented Feb 13, 2025

Not sure how big of an issue is, but the package docs has this:

Fragile modernc.org/libc dependency

When you import this package you should use in your go.mod file the exact same version of modernc.org/libc as seen in the go.mod file of this repository.

See the discussion at https://gitlab.com/cznic/sqlite/-/issues/177 for more details.

In v1.34.1, they use modernc.org/libc@v1.55.3: https://gitlab.com/cznic/sqlite/-/blob/v1.34.1/go.mod?ref_type=tags#L10

@rodrigozhou rodrigozhou changed the title Use modernc.org/sqlite version v1.34.0 Use modernc.org/sqlite version v1.34.1 Feb 13, 2025
@prathyushpv
Copy link
Contributor Author

Not sure how big of an issue is, but the package docs has this:

Fragile modernc.org/libc dependency

When you import this package you should use in your go.mod file the exact same version of modernc.org/libc as seen in the go.mod file of this repository.
See the discussion at https://gitlab.com/cznic/sqlite/-/issues/177 for more details.

In v1.34.1, they use modernc.org/libc@v1.55.3: https://gitlab.com/cznic/sqlite/-/blob/v1.34.1/go.mod?ref_type=tags#L10

Thanks @rodrigozhou for finding that! I have pinned libc to this version as well.

@prathyushpv prathyushpv enabled auto-merge (squash) February 19, 2025 18:25
@prathyushpv prathyushpv merged commit 9bd58f9 into main Feb 19, 2025
50 checks passed
@prathyushpv prathyushpv deleted the ppv/sqlite_version branch February 19, 2025 18:42
rodrigozhou pushed a commit that referenced this pull request Feb 20, 2025
## What changed?
Use version v1.34.1 of modernc.org/sqlite library.

## Why?
We had fixed an issue with contexts being cancelled during a
transaction. But a later change in this library reintroduced this issue.
While we work with the maintainer to fix this, use the previous version.

## How did you test it?

## Potential risks

## Documentation

## Is hotfix candidate?
No
yuandrew added a commit to temporalio/cli that referenced this pull request Mar 28, 2025
## What was changed
<!-- Describe what has changed in this PR -->
Pinned v1.34.1 for modernc/sqlite

## Why?
<!-- Tell your future self why have you made these changes -->
1.34.2 has a regression, waiting on
https://gitlab.com/cznic/sqlite/-/issues/196 to be resolved.
See temporalio/temporal#7333 for more details

## Checklist
<!--- add/delete as needed --->

1. Closes #777

2. How was this tested:
<!--- Please describe how you tested your changes/how we can test them
-->

3. Any docs updates needed?
<!--- update README if applicable
      or point out where to update docs.temporal.io -->
chaptersix pushed a commit to temporalio/cli that referenced this pull request Dec 17, 2025
## What was changed
Wiring in new enableReplication flag into cluster upsert cli.

Deleting sql pinning test as we no longer need this, see historical
context:

#784
temporalio/temporal#7333

then unpinning in OSS temporalio/temporal#8489

## Why?
Allows for users to leverage the new field

## Checklist

1. Closes 

2. How was this tested:
built locally and tested in conjunction with
temporalio/temporal#8744, used the cli manually
to make a replication based test pass
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