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

update deps #5119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

update deps #5119

wants to merge 1 commit into from

Conversation

ognevny
Copy link
Contributor

@ognevny ognevny commented Nov 17, 2023

fixes #5116
just for interest, is it good to update other dependencies?
(hope lock file update doesn't break anything. tested only on x86_64-pc-windows-gnu toolchain)

@ognevny
Copy link
Contributor Author

ognevny commented Nov 17, 2023

oh, I see the CI results. will try to work on it later

@fanninpm
Copy link
Contributor

The CI failures seem unrelated.

@ognevny
Copy link
Contributor Author

ognevny commented Nov 18, 2023

oh, I see the CI results. will try to work on it later

should be fixed. tested on Linux machine

@ognevny
Copy link
Contributor Author

ognevny commented Nov 18, 2023

the Redox failure is very strange

@ognevny
Copy link
Contributor Author

ognevny commented Dec 27, 2023

now not all yanked deps are updated :(

@youknowone
Copy link
Member

@ognevnydemon Oh, I am sorry. This patch was independently updating Cargo.lock, right? I will try to lookup previous version.

@ognevny
Copy link
Contributor Author

ognevny commented Dec 27, 2023

@ognevnydemon Oh, I am sorry. This patch was independently updating Cargo.lock, right? I will try to lookup previous version.

yeah, but I just ran cargo update which is not correct in many cases... so maybe it's needed to update them carefully

@youknowone youknowone force-pushed the update-yanked-deps branch 2 times, most recently from 65c37ff to a71608b Compare December 27, 2023 18:42
@youknowone
Copy link
Member

youknowone commented Dec 27, 2023

just for interest, is it good to update other dependencies?

Yes, we update dependencies for various reasons.
Without getting new features, these are good reasons to update dependencies.

  • To remove one of multiple versions of same library in Cargo.lock
  • To follow up incompatible library update. (major upgrade for >= 1.0.0 or minor upgrade for <1.0.0)

Avoiding yanked version is one of the good reason to do it. Any other reasonable suggestion will be also welcome.

@ognevny
Copy link
Contributor Author

ognevny commented Dec 27, 2023

Yes, we update dependencies for various reasons.
Without getting new features, these are good reasons to update dependencies.

  • To remove one of multiple versions of same library in Cargo.lock
  • To follow up incompatible library update. (major upgrade for >= 1.0.0 or minor upgrade for <1.0.0)

Avoiding yanked version is one of the good reason to do it. Any other reasonable suggestion will be also welcome.

understood. thanks for attention! maybe I'll try to update more deps if they have more fresh version

@youknowone
Copy link
Member

uh.. openssl has changes

@youknowone
Copy link
Member

which crate would affect test_fractions?

@youknowone youknowone mentioned this pull request Apr 8, 2024
@youknowone
Copy link
Member

maybe num-complex or other num- one

@ognevny
Copy link
Contributor Author

ognevny commented Apr 11, 2024

I rebased the changes, now this PR updates only lockfile

@ognevny ognevny changed the title update yanked deps update deps Apr 11, 2024
@youknowone
Copy link
Member

The change is partially merged in #5217

@ognevny
Copy link
Contributor Author

ognevny commented Apr 11, 2024

looks like only one test fails. is this critical?

@ognevny
Copy link
Contributor Author

ognevny commented Apr 11, 2024

The change is partially merged in #5217

yes, I rebased my branch to main so now it's just cargo update change

@youknowone
Copy link
Member

It is hard to say critical, but at the same time, updating deps is not also critical.
Merging without reasoning doesn't seem reasonable.
It will be worth to try revert only num-* crates and retrying it to check what's going on.

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.

update yanked dependencies
3 participants