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

Make rusqlite optional #4791

Merged
merged 12 commits into from
May 17, 2024
Merged

Make rusqlite optional #4791

merged 12 commits into from
May 17, 2024

Conversation

hugocaillard
Copy link
Collaborator

@hugocaillard hugocaillard commented May 14, 2024

This PR is part of an initiative to reduce the size and the impact of #4756.

Description

Isolate the rusqlite dependency in stacks-common and clarity behind a cargo feature.
This allows to use the clarity-vm without the rusqlite dependency, as it's done in Clarinet.

This PR adds a canonical cargo feature to stacks-common and clarity.
Ultimately, the cargo features of these 2 Cargo.toml will get more complex, as they are in #4756 (see here and there). But compared to the other PR, I didn't add a extra sqlite feature which appears to be useless.

Question:

  • does the cargo feature management seem ok? (knowing that it will get more complex)

To make sure that the PR works as expected, you can run

> cargo check -p clarity
> cargo check -p clarity  --no-default-features --features=developer-mode
# I may want to add that to the CI

And make sure that rusqlite isn't pulled in clarity

> cargo tree -i rusqlite -p clarity  --no-default-features --features=developer-mode

(should output error: package ID specification rusqlite did not match any packages)

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@hugocaillard hugocaillard marked this pull request as ready for review May 14, 2024 17:38
@obycode obycode requested review from obycode and a team May 14, 2024 18:52
obycode
obycode previously approved these changes May 15, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM!

@hugocaillard hugocaillard requested a review from a team as a code owner May 15, 2024 18:45
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM; just a couple nits to address before approving

@jcnelson
Copy link
Member

Also, I think you should update CI with these cargo check statements so we can be certain that Clarity remains isolated from rusqlite.

kantai
kantai previously approved these changes May 16, 2024
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM, just address Jude's comments

@hugocaillard hugocaillard dismissed stale reviews from kantai and obycode via 35f2401 May 16, 2024 09:13
@hugocaillard
Copy link
Collaborator Author

@wileyj I'd like to add cargo check -p clarity --no-default-features --features=developer-mode somewhere in the CI, what would be the best place to do so?

Note: in the feature we'll also add checks to make sure that clarity compile to --target wasm32-unknown-unknown, it'll require some more work to get there

@wileyj
Copy link
Contributor

wileyj commented May 16, 2024

@wileyj I'd like to add cargo check -p clarity --no-default-features --features=developer-mode somewhere in the CI, what would be the best place to do so?

Note: in the feature we'll also add checks to make sure that clarity compile to --target wasm32-unknown-unknown, it'll require some more work to get there

can you walk me through what you want to do here? is the idea to adjust a current workflow, or are we adding a new one?
more than likely, this would need to happen in the https://github.com/stacks-network/actions repo vs the stacks-core repo

@hugocaillard
Copy link
Collaborator Author

hugocaillard commented May 16, 2024

@wileyj Add a new one. We want to make sure that the clarity package passes the check with no default feature and the developer-mode feature https://github.com/stacks-network/stacks-core/blob/feat/sqlite-feature-flag/clarity/Cargo.toml#L58

We can probably do that outside of this PR i guess

@wileyj
Copy link
Contributor

wileyj commented May 16, 2024

@wileyj Add a new one. We want to make sure that the clarity package passes the check with no default feature and the developer-mode feature https://github.com/stacks-network/stacks-core/blob/feat/sqlite-feature-flag/clarity/Cargo.toml#L58

We can probably do that outside of this PR i guess

got it - yes, this can be addressed in that actions repo so not a blocker on this PR. i'll ping you outside of this PR

@hugocaillard
Copy link
Collaborator Author

@jcnelson

I think it's better to do the CI work in an other PR. Maybe even after we can compile clarity to wasm

kantai
kantai previously approved these changes May 16, 2024
obycode
obycode previously approved these changes May 16, 2024
@hugocaillard hugocaillard dismissed stale reviews from obycode and kantai via 951c479 May 17, 2024 08:03
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

👍

@obycode obycode added this pull request to the merge queue May 17, 2024
Merged via the queue into develop with commit cf01028 May 17, 2024
1 check passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants