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

Updated dependencies #432

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Updated dependencies #432

wants to merge 4 commits into from

Conversation

ivanrg99
Copy link

@ivanrg99 ivanrg99 commented Jul 2, 2024

Updated some of the dependencies on the project

@ivanrg99
Copy link
Author

env_logger and other packages (like syn) will fail due to the MSRV being 1.60, which is quite old at this point. We should consider increasing the MSRV

@daniel-abramov
Copy link
Member

We once had to downgrade it #432, but maybe it's not an issue anymore.

And yeah, I believe that this one #411 fails exactly because of this.

@daniel-abramov
Copy link
Member

I've raised MSRV to match that of tokio-tungstenite (kind of makes sense if both of them use the same MSRV). Could you rebase your PR to check if it builds?

Another question - why did you remove the patch from the dependency versions? - E.g. log = "0.4.8" to log = "0.4". AFAICR the latter is just a shortcut for log = "0.4.0" which is more flexible but makes builds a bit less reproducible in the sense that it would allow the version "0.4.1" which may not be desirable given that "0.4.8" might have fixed a security issue or included a patch (as a general idea).

@ivanrg99
Copy link
Author

You are totally right about the missing patch version. I though it was a shortcut for the latest patch version available, but I doubled check the docs and you are correct. I fixed it in my local branch.

As to bumping the MSRV from 1.60.0 to 1.63.0, this still poses problems. CI only runs a cargo check to see if everything is fine, but version 1.63 is unable to compile the tests (with all the dependencies being updated, that is). I have tried different combinations, and the lowest MSRV that I can get to pass clean (including running all tests using that version, using all possible feature combinations, and passing with all the latest dependencies) is 1.65.0. The tradeoff is that for 1.65.0 to be able to run the tests, some of the dev dependencies can't be upgraded. The only two affected are criterion (stuck in 0.3.6) and env_logger (stuck in 0.10.2). I think that just using a cargo check as a means to validate the MSRV is not that great, since we can't really run the tests to make sure if works. Ultimately it's up to whatever the project needs/decides. I just see that most of the Github Actions that check the MSRV fail, so maybe it's worth bumping that number of to 1.71.0

As an anecdote, I ran the benchmarks with all the dependencies updated to their latest versions, and I saw some performance regressions in my machine (M3 Max) on the InputBuffer and ReadBuffer (heap) benchmarks. I still think it is worth upgrading, since security and bug fixes/correctness should be the main priority, but still interesting to point out.

Let me know what you decide

@daniel-abramov
Copy link
Member

You are totally right about the missing patch version. I though it was a shortcut for the latest patch version available, but I doubled check the docs and you are correct. I fixed it in my local branch.

Would you revert the aforementioned changes then? :) So that at least we have a bit cleaner diff with versions that were actually updated.

I just see that most of the Github Actions that check the MSRV fail, so maybe it's worth bumping that number of to 1.71.0

Right, 1.71.0 is the MSRV of the env_logger. Truthfully, 1.71.0 is more than a year old, so it is a reasonable choice. However, I don't know how many projects would benefit from having a much lower MSRV. I don't have a strong opinion on MSRV, so I would be totally fine with 1.71.0. Unless anyone voices a strong concern against that, we could increase MSRV.

As an anecdote, I ran the benchmarks with all the dependencies updated to their latest versions, and I saw some performance regressions in my machine (M3 Max) on the InputBuffer and ReadBuffer (heap) benchmarks.

How much though? Is it a significant difference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants