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

Bump depenencies (metrics) #2061

Closed
wants to merge 2 commits into from
Closed

Conversation

LecrisUT
Copy link
Contributor

A quick dependency bump request to be compatible with the Fedora reviews

  • Relaxed the sqlx dependency. Any reasons why it was pinned?
  • metrics dependencies bump. It seems the compilation now fails. I didn't look into the changelog to see how it should be fixed

About dependabot, maybe you should consider renovate instead. One main issue with dependabot is that it continuously closes and opens PRs, even if you use the new group feature. renovate on the other hand would always group and accumulate the dependency changes and rebase everytime on the same PR(s). This is helpful when you have multiple projects being closely linked to each other.

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

@ellie
Copy link
Member

ellie commented May 30, 2024

No particular reason for it being pinned, likely part of past debugging

Would you be able to fix the metrics error?

What you describe as an issue with dependabot is actually something I quite like. I find it preferable to review each dependency at a time, and ensure that the changelog is properly reviewed - this is unlikely to happen with one giant change.

Why is this required for Fedora?

@LecrisUT
Copy link
Contributor Author

Would you be able to fix the metrics error?

I mostly wanted to open the issue to show the errors. If you can fix them, please go ahead. I am not proficient in rust, I just monkey around in it, and throw stuff at it until the IDE shows me something useful.

What you describe as an issue with dependabot is actually something I quite like

Understandable. I personally prefer to get all dependencies resolved together. You still get each individual changelog when these are grouped, but you are also able to pick up changes that are dependent on each other. If you want to check individual changes, renovate would also allow you to do that by selecting which update you want in the PR.

Why is this required for Fedora?

Are you referring to how the changes of this PR are needed for Fedora?

The relaxing of dependency is always useful, because otherwise we are making patches to do that on our side, which can be messy to maintain. In Fedora packaging, there is only one set of package available for each rust package, so there is no freedom like with Flatpak to use the exact dependencies specified in the project. So we often have to change the dependencies written there. If we see that the package still builds and tests pass with the current set of dependencies, than we go ahead with the packaging and wait for upstream/dependencies to catch up. But every so often there are breaking changes and we try to work together to make the necessary patches (there are people more rust proficient who get involved).

This has the side-effect, that you get a bit more nudging from us whenever there are important dependency updates to bump 😉

@tessus
Copy link
Contributor

tessus commented May 30, 2024

I can work on fixing the metrics errors. 2 options: I can

- work on it in this PR (I need push access on the OP's repo for this)
- create a separate PR that is based on this one

P.S.: I'll create a PR based off this one later this evening.

@ellie
Copy link
Member

ellie commented May 30, 2024

Yeah the metrics dependabot PR is failing, so it's already visible. I'm more questioning whether updating that specific package is required for fedora or not?

Fedora packaging, there is only one set of package available for each rust package

Are you saying that there is only one version of each dependency that we can use for fedora?

We're currently in several other distros and package managers with no such issues.

I'm more than happy to make reasonable changes in order to make maintainers lives easier, but that doesn't sound particularly sustainable to me. What if we require a version of a dependency that fedora does not support?

@LecrisUT
Copy link
Contributor Author

LecrisUT commented May 30, 2024

I can work on fixing the metrics errors. 2 options: I can

- work on it in this PR (I need push access on the OP's repo for this) - create a separate PR that is based on this one

P.S.: I'll create a PR based off this one later this evening.

Sure, pls go ahead.

Are you saying that there is only one version of each dependency that we can use for fedora?

Yes, that's the case for most non-atomic distros. It may depend on the specific distro's guidelines, but thr general case is everything is shared and concise.

What if we require a version of a dependency that fedora does not support?

We always strive to update to the latest version as quickly as possible. If you update to a version newer than we have, we will follow suite and update them and the other dependants. If the dependencies are still compatible, which we check by the bundled tests, we silently make the changes. Otherwise we contribute patches upstream.

Another model is that compatibility versions of dependencies are preserved. This however are rather special cases, and we instead strive to work with upstream first.

but that doesn't sound particularly sustainable to me

For packagers this is our bread and butter and we are used to it. We know that for a user the vendoring approach is more simple, but this model can be more beneficial for the whole ecosystem to spread updates and fixes, to promote active alternatives and so on.

For the upstream developers side, you are not alone in this. I am primarily a C++/Python packager so I can't propose fixes as quickly, but usually we come with ready-fixes when it comes to bumping dependencies, although for tests it can be more complicated.

I am pushing through packaging atuin because I really like using it and would like to make it more accessible, even if it means packaging 40+ dependencies. Once this is done though I will contribute some systemd units for the server package.

@ellie
Copy link
Member

ellie commented May 31, 2024

If you update to a version newer than we have, we will follow suite and update them and the other dependants. If the dependencies are still compatible, which we check by the bundled tests, we silently make the changes. Otherwise we contribute patches upstream.

Thanks for clarifying!

Sorry for any potentially perceived pushback, but I found the approach to handling dependencies initially concerning, and just want to make sure I understand the implications of how you do things correctly

So far, I've only worked with packagers that use either nix or rely on cargo for rust dependencies.

I am pushing through packaging atuin because I really like using it and would like to make it more accessible, even if it means packaging 40+ dependencies. Once this is done though I will contribute some systemd units for the server package.

Thanks for this! Glad to hear you enjoy using it 😊

@LecrisUT
Copy link
Contributor Author

Thanks for clarifying!

Sorry for any potentially perceived pushback, but I found the approach to handling dependencies initially concerning, and just want to make sure I understand the implications of how you do things correctly

No worries at all, I've found the exchange very fruitful. I very much appreciate it when upstream gets involved in these topics.

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.

3 participants