-
Notifications
You must be signed in to change notification settings - Fork 315
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
feat: upgrade some dependencies #1126
Conversation
Could someone review and merge this PR? |
Thanks for your PR. We are a bit hesitate to merge it right away as this repo is really a core piece of Filecoin. It will take us some time to look into all the implications those dependency updates have. But it will save us time as we can just take your changes, once we decide on an upgrade. Do you know if the test failure is related to your change or whether it's unrelated? |
The rust stable version in CI is too old. |
I've assigned this one to me. I'll first tackle #1142 and then look over the upgraded dependencies and look into the impact. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is me review. Updating all those dependencies is fine. Though I'd like to wait until #1149 is merged, so that we can have all tests pass on CI.
Here's the long version:
-
git2: We only use basic functionality, there shouldn't be any issues.
-
heim: Upgrade make sense as we now use async-std instead of futures-preview which is an outdated thing.
-
uom: little changes, mostly new stuff, I don't see a problem with that.
-
serde_cbor: Got a bunch of fixes.
-
env_proxy: updated underlying url library which sounds like a good idea.
-
simplelog: only used in phase2 tool. The logging output changed a bit, there is no padding around the level. That's not a problem, I checked with @DrPeterVanNostrand). New version needs less unwrapping, which is nice.
-
reqwest: only used in paramfetch. We use basic functionality only, improvements are always welcome. Also upgraded underlying url library (like env_proxy did).
-
rexpect: used for testing only. Various improvements like better error messages.
-
config: it doesn't do automatic lowercasing of keys in TOML files any more. But that shouldn't be a problem as we don't access the configuration directly, but transform it into the
Settings
struct. -
proptest: is used for testing only. I think improvements there are always welcome.
-
femme: only appears in commented out code in tests. So it can't really break things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koushiro As we now use a newer version of stable Rust, the tests should pass. Could you please rebase your changes. Ideal would be if you put all your changes into a single commit and rebase that on master. In case you don't have the time, please let me know and I'll make those changes to this PR. |
@vmx PTAL |
In my previous comment I forgot to mention that changing from a fixed version of This PR needs another rebase. But I'd wait for that until we are right before the merge, else we keep rebasing all the time. @cryptonemo This PR looks good to me. Please let @koushiro know when merging it comes close, so that he can rebase it one last time (as before I'm happy to do the rebase as well). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koushiro This looks good, sorry for the delay on review. I'm planning to test and rebase it and merge if all is well at some point today. If you rebase first, all the better. Thanks!
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
Signed-off-by: koushiro koushiro.cqx@gmail.com