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

fix(install): Downgrade owo-colors from 3.6.0 to 3.5.0 #6203

Merged
merged 2 commits into from
Feb 23, 2023
Merged

Conversation

oxarbitrage
Copy link
Contributor

Motivation

owo-colors v 0.3.6 is yanked. This PR downgrade to version 0.3.5. Close #6197

Solution

Downgrade.

Test the warning is gone with:

cargo install --locked --features getblocktemplate-rpcs --git https://github.com/ZcashFoundation/zebra --branch issue6197 zebrad

Dependabot will try to update, we will deny, dependabot will leave us alone until a greater version is released.

The change in command.rs is needed to fix a clippy lint that was not happening when owo-colors was at 0.3.6.

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@oxarbitrage oxarbitrage requested a review from a team as a code owner February 22, 2023 15:40
@oxarbitrage oxarbitrage requested review from teor2345 and removed request for a team February 22, 2023 15:40
@github-actions github-actions bot added C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Feb 22, 2023
@oxarbitrage oxarbitrage self-assigned this Feb 22, 2023
@oxarbitrage oxarbitrage added A-dependencies Area: Dependency file updates P-Medium ⚡ labels Feb 22, 2023
@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Merging #6203 (4a80946) into main (4daedbc) will decrease coverage by 0.10%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6203      +/-   ##
==========================================
- Coverage   78.01%   77.92%   -0.10%     
==========================================
  Files         304      304              
  Lines       39251    39251              
==========================================
- Hits        30623    30586      -37     
- Misses       8628     8665      +37     

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I'm surprised to see such a large change to Cargo.lock. How did you generate it?

We're keeping some packages to specific versions, and there are some cryptographic package changes in here. So I think we might need to check each dependency change in detail to make sure they are correct. Can you split those changes into another PR, and just to the owo-colors change in this one?

zebra-test/tests/command.rs Outdated Show resolved Hide resolved
deny.toml Outdated Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor Author

I'm surprised to see such a large change to Cargo.lock. How did you generate it?

I just downgraded owo-colors and build, Cargo.lock is auto generated.

We're keeping some packages to specific versions, and there are some cryptographic package changes in here. So I think we might need to check each dependency change in detail to make sure they are correct. Can you split those changes into another PR, and just to the owo-colors change in this one?

I am not understanding, changes here are just to make the downgrade work. I can't see unrelated changes. Deny.toml changes are done to make the CI pass.

@teor2345
Copy link
Contributor

I'm surprised to see such a large change to Cargo.lock. How did you generate it?

I just downgraded owo-colors and build, Cargo.lock is auto generated.

Cargo.lock is auto-generated from the Cargo.toml files and the previous Cargo.lock. It's easy to accidentally change it a lot. It can happen if you delete the Cargo.lock, replace it with an older version, or use a command like cargo update to update all the dependency versions.

We're keeping some packages to specific versions, and there are some cryptographic package changes in here. So I think we might need to check each dependency change in detail to make sure they are correct. Can you split those changes into another PR, and just to the owo-colors change in this one?

I am not understanding, changes here are just to make the downgrade work. I can't see unrelated changes. Deny.toml changes are done to make the CI pass.

There are 700 lines of changes in Cargo.lock. That's a very big change.

For example, the cryptographic dependency bls12_381 should not change in a PR about a terminal colour dependency:
https://github.com/ZcashFoundation/zebra/pull/6203/files#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87eR484-R488

Screenshot 2023-02-23 at 07 21 24

@teor2345
Copy link
Contributor

Happy to jump on a call if that would help?

@oxarbitrage
Copy link
Contributor Author

oxarbitrage commented Feb 22, 2023

Happy to jump on a call if that would help?

Sorry, it seems i screwed up somewhere, my apologies for not checking it carefully.

It seems only a downgrade in the Cargo.toml is needed, no changes were done to the lock after built. The warning is gone and no need to change deny.toml.

@oxarbitrage
Copy link
Contributor Author

Hmm, unfortunatly the warning is still there, so it needs further changes.

$ cargo install --locked --features getblocktemplate-rpcs --git https://github.com/ZcashFoundation/zebra --branch issue6197 zebrad
    Updating git repository `https://github.com/ZcashFoundation/zebra`
  Installing zebrad v1.0.0-rc.4 (https://github.com/ZcashFoundation/zebra?branch=issue6197#35199d2e)
    Updating crates.io index
warning: package `owo-colors v3.6.0` in Cargo.lock is yanked in registry `crates-io`, consider running without --locked

@oxarbitrage
Copy link
Contributor Author

Trying updated Cargo.lock after cargo update -p owo-color in 4a80946

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, that looks a lot better!

mergify bot added a commit that referenced this pull request Feb 23, 2023
@mergify mergify bot merged commit b6f5164 into main Feb 23, 2023
@mergify mergify bot deleted the issue6197 branch February 23, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider downgrading owo-colors crate?
2 participants