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

Move blockstack-core into a separate folder (develop branch) #3814

Merged
merged 10 commits into from
Aug 3, 2023

Conversation

sergey-shandar
Copy link
Contributor

@sergey-shandar sergey-shandar commented Jul 28, 2023

Description

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

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #3814 (e4a9632) into develop (b820596) will decrease coverage by 0.02%.
Report is 4 commits behind head on develop.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3814      +/-   ##
===========================================
- Coverage     0.18%    0.16%   -0.02%     
===========================================
  Files          306      305       -1     
  Lines       280746   280694      -52     
===========================================
- Hits           512      469      -43     
+ Misses      280234   280225       -9     
Files Changed Coverage Δ
clarity/src/vm/types/serialization.rs 0.00% <0.00%> (ø)
stackslib/src/blockstack_cli.rs 0.00% <0.00%> (ø)
stackslib/src/burnchains/affirmation.rs 0.00% <ø> (ø)
stackslib/src/burnchains/bitcoin/address.rs 0.00% <ø> (ø)
stackslib/src/burnchains/bitcoin/bits.rs 0.00% <ø> (ø)
stackslib/src/burnchains/bitcoin/blocks.rs 0.00% <ø> (ø)
stackslib/src/burnchains/bitcoin/indexer.rs 0.00% <ø> (ø)
stackslib/src/burnchains/bitcoin/mod.rs 0.00% <ø> (ø)
stackslib/src/burnchains/bitcoin/network.rs 0.00% <ø> (ø)
stackslib/src/burnchains/bitcoin/spv.rs 0.00% <ø> (ø)
... and 132 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sergey-shandar sergey-shandar changed the title Move blockstack-core into a separate folder Move blockstack-core into a separate folder (develop branch) Jul 28, 2023
@sergey-shandar sergey-shandar linked an issue Jul 28, 2023 that may be closed by this pull request
@sergey-shandar sergey-shandar self-assigned this Jul 28, 2023
@sergey-shandar
Copy link
Contributor Author

@kantai, @jcnelson, @stjepangolemac the CI is passing now for the PR.

Copy link
Contributor

@stjepangolemac stjepangolemac left a comment

Choose a reason for hiding this comment

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

Looks good. Do you think we could fit changing dependencies to workspace ones in this PR? Maybe it's better to leave that for another day.

@jcnelson
Copy link
Member

jcnelson commented Aug 3, 2023 via email

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.

See earlier emailed comment

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.

The only changes to Cargo.lock should be changing blockstack-core to stackslib.

@sergey-shandar
Copy link
Contributor Author

sergey-shandar commented Aug 3, 2023

The only changes to Cargo.lock should be changing blockstack-core to stackslib.

How often do we update Cargo.lock? Do we have separate PRs for this type of changes? Or are we stuck with the old versions of packages?

@AshtonStephens here's another example of Cargo.lock in the repo.

@jcnelson
Copy link
Member

jcnelson commented Aug 3, 2023

How often do we update Cargo.lock? Do we have separate PRs for this type of changes? Or are we stuck with the old versions of packages?

We change versions when it's necessary to make progress or to fix disclosed vulnerabilities. I make it a point to look through the code for each dependency so I know what we're responsible for maintaining.

@sergey-shandar
Copy link
Contributor Author

sergey-shandar commented Aug 3, 2023

Looks good. Do you think we could fit changing dependencies to workspace ones in this PR? Maybe it's better to leave that for another day.

I would make it as a separate PR(s). Issue #3834

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.

Overall LGTM now, but please make that one change about opt-level before merging. Thanks!

@sergey-shandar sergey-shandar merged commit 7afb430 into develop Aug 3, 2023
1 check passed
@sergey-shandar sergey-shandar deleted the sergey/3809-workspace-develop branch August 3, 2023 23:08
@kantai
Copy link
Member

kantai commented Aug 4, 2023

Hmm.

This PR radically slowed down the unit test time -- went from ~40 minutes to now about 100 minutes.

@sergey-shandar
Copy link
Contributor Author

Hmm.

This PR radically slowed down the unit test time -- went from ~40 minutes to now about 100 minutes.

It could be related to the opt-level #3835

@sergey-shandar
Copy link
Contributor Author

sergey-shandar commented Aug 4, 2023

Hmm.

This PR radically slowed down the unit test time -- went from ~40 minutes to now about 100 minutes.

@kantai check this PR #3838 if it fixes the issue.

@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 Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Top level Cargo.toml file shouldn't have package properties.
5 participants