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

Workspace level profiles #3838

Merged
merged 2 commits into from
Aug 7, 2023
Merged

Workspace level profiles #3838

merged 2 commits into from
Aug 7, 2023

Conversation

sergey-shandar
Copy link
Contributor

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 Aug 4, 2023

Codecov Report

Merging #3838 (c356b84) into develop (b820596) will decrease coverage by 0.02%.
Report is 5 commits behind head on develop.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3838      +/-   ##
===========================================
- 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

Cargo.toml Outdated
Comment on lines 21 to 22
[profile.dev.package.regex]
opt-level = 2
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop this specification, since it gets set to 3 above anyways.

@kantai
Copy link
Member

kantai commented Aug 4, 2023

@sergey-shandar -- thanks for opening this. I also opened a similar PR #3836.

I don't think we should actually remove the profile settings from the workspace members yet, though. clarity, in particular, can be published by itself, and in that case, the profile settings from its Cargo file are actually used. There's a long-standing open issue about that cargo warning here (rust-lang/cargo#8264), and I'm inclined to just let that warning continue to echo in our case, because we do often need to define workspace member profiles

@sergey-shandar
Copy link
Contributor Author

sergey-shandar commented Aug 4, 2023

@sergey-shandar -- thanks for opening this. I also opened a similar PR #3836.

I don't think we should actually remove the profile settings from the workspace members yet, though. clarity, in particular, can be published by itself, and in that case, the profile settings from its Cargo file are actually used. There's a long-standing open issue about that cargo warning here (rust-lang/cargo#8264), and I'm inclined to just let that warning continue to echo in our case, because we do often need to define workspace member profiles

It depends how do you build clarity or any other packages. For example, if you build and publish it from a repository root, then it will always use workspace settings. Also, if you reference clarity using git URL clarity = { git = "https://github.com/stacks-network/stacks-blockchain/", branch = "develop" } it will also use workspace settings.

@kantai
Copy link
Member

kantai commented Aug 4, 2023

@sergey-shandar -- thanks for opening this. I also opened a similar PR #3836.

I don't think we should actually remove the profile settings from the workspace members yet, though. clarity, in particular, can be published by itself, and in that case, the profile settings from its Cargo file are actually used. There's a long-standing open issue about that cargo warning here (rust-lang/cargo#8264), and I'm inclined to just let that warning continue to echo in our case, because we do often need to define workspace member profiles

It depends how do you build clarity or any other package. For example, if you build and publish it from a repository root, then it will always use workspace settings. Also, if you reference clarity using git URL clarity = { git = "https://github.com/stacks-network/stacks-blockchain/", branch = "develop" } it will also use workspace settings.

Ah, in that case, I think that's fine.

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.

This LGTM, but please drop the regex specific opt-level setting

@sergey-shandar sergey-shandar marked this pull request as ready for review August 4, 2023 23:56
@sergey-shandar
Copy link
Contributor Author

@kantai could you make sure that this PR is fixing an issue with CI time?

@kantai kantai merged commit 4e411e1 into develop Aug 7, 2023
2 checks passed
@sergey-shandar sergey-shandar deleted the 3835-workspace-opt-level branch August 7, 2023 15:38
@sergey-shandar sergey-shandar linked an issue Aug 7, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Set opt-level on workspace level
3 participants