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

WIP: Add Pub/Dart support #3438

Closed

Conversation

IchordeDionysos
Copy link

@IchordeDionysos IchordeDionysos commented Apr 3, 2021

This PR adds support for Dart's package manager pub.

Various examples for Dart projects can be found here: https://github.com/IchordeDionysos/pub_examples

Help in form of PRs to simpleclub-extended:project/pub-dart are welcome 😍 .

Fixes #2166

Status of the PR

All required parts are already implemented, I am currently working on properly adding all boilerplate used for the project setup and CI/CD. 🚀

The implementation is in parts still a bit rough and I want to improve this. For that, I am already in contact with the Dart team to find better solutions or to get the current implementation officially supported and finalized.

I'll try to get instructions ready on how we can properly end-to-end test the Dependabot pub implementation and how we can use it until it finally gets merged into the Dependabot repo.


The following parts are already implemented and unit tested:

P.S. I am aware that currently, the PR will not land in the official repo. I just wanted to make the work more visible to potential contributors and the dependabot team.

@rolandgeider
Copy link

Can I help testing or something?

@IchordeDionysos
Copy link
Author

IchordeDionysos commented Apr 4, 2021

@rolandgeider the PR is currently in an early state where not all required parts are implemented yet.

Due to this, I am only working with unit tests.

I am currently working on the update_checker and will continue to work on it tomorrow.

As you can see in the original message there are only three parts left UpdateChecker (working on it), FileUpdater (I made some changes there already, but a few things are still to do), MetadataFinder (not work yet, the only exploration of potential implementation).

Everything is now finished, underlying implementations might change based on feedback.


I think where you could help right now is to implement the MetadataFinder.

For implementing this we basically only have to git a Git provider URL, like https://github.com/dependabot/dependabot-core, as described here:
https://github.com/dependabot/dependabot-core/blob/main/common/lib/dependabot/metadata_finders/README.md

My current thoughts on how to get the URL are to read it from the pubspec.yaml file contents and take the URL from repository (if present) if not use homepage (if present and a Git Provider URL).
See also: dart-lang/pub-dev#4696

P.S.: Don't be confused if you find references to Terraform as I used the Terraform implementation as a quickstart.

MetadaFinder is implemented 💪


Would be happy to have any help I can get once the implementation is finished 😍

@IchordeDionysos
Copy link
Author

IchordeDionysos commented Apr 11, 2021

@JohannSchramm I like how clean your implementation is 😍

What I am currently strugeling is to decide what the ultimately best implementation for Pub support is...

I see multiple options we could take and every would have some downsides:

  • Directly implementing version/requirement parsing/updating (like you did).
    Downside: Having to reimplement the version resolving as described here Pub Version solver which would be huge and the resulting pubspec.yaml file might be incompatible.
  • Relying on dart pub outdated with some manual work to update the requirements and pubspec files. (What I did in my PR)
    Downside: Fixing breaking changes may still have to be fixed. Also there might be some incompatibilities if transitive dependencies change in newer versions.
  • Relying on dart pub outdated and dart pub upgrade with minimal work on the pub side.
    Downside: Pub currently is not optimized to upgrade only one dependency and only works well for upgrading all dependencies. However Dependabot does not support multi-dependency upgrades (yet?).

I had huge a discussion on the Flutter/Dart Discord with @jonasf on what might be the best approach and he strongly suggested to used dart pub outdated and dart pub upgrade and I agree with him that due to the complexity of transitive dependencies and dependency incompatibilities it would be very complicated to re-write this whole logic.

Would be happy to collaborate on this here.

@jonasfj
Copy link
Contributor

jonasfj commented May 3, 2021

Downside: Pub currently is not optimized to upgrade only one dependency and only works well for upgrading all dependencies.

This isn't just a pub thing. Pub can actually do it, dart pub upgrade <package> and dart pub upgrade --major-versions <package> both works. But: sometimes it's simply not possible to upgrade package foo without also upgrading package bar. So whatever we do, we have to handle that scenario.


@IchordeDionysos, @JohannSchramm, if you have ideas for tweaks we should make to dart pub outdated --json or dart pub upgrade [--major-versions] [<package>...] that would help implement dependabot integration please file a bug in the pub repository and ping me on the issues. Given that dependabot isn't currently accepting new eco-systems we could use the time to add any features we need in dart pub :D

@jurre
Copy link
Member

jurre commented May 3, 2021

But: sometimes it's simply not possible to upgrade package foo without also upgrading package bar. So whatever we do, we have to handle that scenario.

That's the case for most package managers, if those packages are transitive dependencies and not pulled in as top-level, dependabot will always just update them, for top-level dependencies we raise an error in most scenario's and attempt to explain why we could not perform the update.

Copy link

@Stargator Stargator left a comment

Choose a reason for hiding this comment

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

Not familiar with Dependabout to provide comments related to the work tied to that, but I have left some comments and questions regarding the dart side of things.

pub/README.md Outdated Show resolved Hide resolved
pub/helpers/pubspec.yaml Outdated Show resolved Hide resolved
pub/helpers/pubspec.yaml Outdated Show resolved Hide resolved
pub/lib/dependabot/pub/metadata_finder.rb Show resolved Hide resolved
headers: {
accept: "application/vnd.pub.v2+json",
"X-Pub-Environment": "dependabot"
# TODO: Condier adding X-Pub-Headers (https://github.com/dart-lang/pub/blob/master/doc/repository-spec-v2.md)

Choose a reason for hiding this comment

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

Does this still need to be done?

pub/script/ci-test Outdated Show resolved Hide resolved
@IchordeDionysos
Copy link
Author

@Stargator thanks for commenting on the PR.
Will try to work on the PR on the weekend 👍

Rakefile Show resolved Hide resolved
@IchordeDionysos
Copy link
Author

IchordeDionysos commented May 16, 2021

Thanks for all your great input 😍 @JohannSchramm @jonasfj @jurre @Stargator @mattt

It helped a lot and I finally got a first end-to-end test up and running 💪
See these automatically created PRs:
IchordeDionysos/pub_examples#9
IchordeDionysos/pub_examples#10

There are a few things still missing, but overall I think it would now be super helpful to have some more real-world tests with various projects :)

Still needed:

  • Updating major version (only minor versions are upgraded right now)
  • Caching API requests to the Pub servers (Maybe something to include in the Dart tooling?)
  • Tests have to be updated 🌝
  • Upgrading git dependencies (that's harder now, as we can't integrate too much with things like GitCommitChecker that would support this almost out-of-the-box)
  • Fallback to upgrading all dependencies when there is an issue with upgrading only one dependency. (Is this a normal behavior of dependabot?)

Under the hood, the integration uses the Dart/Pub CLI (pub outdated and pub upgrade).
So there isn't too much magic attached to the dependabot integration.

For an example usage see the README:
https://github.com/IchordeDionysos/dependabot-pub-runner/tree/main

Or look at the this workflow file:
https://github.com/IchordeDionysos/pub_examples/blob/main/.github/workflows/dependabot.yaml

@nstrelow
Copy link

nstrelow commented Jun 2, 2021

Hey, awesome PR and so much looking forward to using this :D

I am using this as suggested inside a github action (dependabot-pub-runner)
For some reason dependabot can't find flutter even though it is installed and I am able to run flutter doctor before upgrading.

Failing action: https://github.com/hpi-studyu/studyu/runs/2731183166?check_suite_focus=true

Sorry if it would've been more appropriate to open a Issue on dependabot-pub-runner.

@IchordeDionysos
Copy link
Author

@nstrelow no, it's fine here I think :)
Forgot to install Flutter in the runner, it should now work. (But haven't tested it myself yet)

If you could quickly retry if it works now that would be awesome!

@nstrelow
Copy link

nstrelow commented Jun 3, 2021

@IchordeDionysos Thanks a lot.
It seems to start working now.

Sadly I have a mono repo with multiple packages. Some dependencies are defined using path: ../package_name.

Is there a way to support this (and just skip those packages).

Or support mono repos in general?

Failing action: https://github.com/hpi-studyu/studyu/runs/2738644494?check_suite_focus=true

@rolandgeider
Copy link

FYI, I tried this on my flutter project and am getting a ParserError, probably because of the analytics message?

Here is the failed run: https://github.com/wger-project/flutter/runs/2889222807?check_suite_focus=true

@nstrelow
Copy link

nstrelow commented Jul 13, 2021

Getting the same error as @rolandgeider (changed my mono repo to use pub.dev hosted to make it work this far).

https://github.com/hpi-studyu/studyu/runs/3059975772?check_suite_focus=true

EDIT: Adding flutter config --no-analytics or --analytics doesn't remove the message somehow.

@jurre jurre added the T: new-ecosystem Requests for new ecosystems/languages label Nov 30, 2021
@fzyzcjy
Copy link

fzyzcjy commented Nov 30, 2021

Hi is there any updates after months? This is very helpful. Thanks!

@Nishnha
Copy link
Member

Nishnha commented Dec 13, 2021

Hi @fzyzcjy, yep! 😄

The Dependabot team is not currently able to take on the maintainer role for new ecosystems, as we outlined in our readme.

That said, we are working with the Dart team on adding Pub support to Dependabot! 🎉
The Dart team has already started a WIP PR.

The way this will be implemented will look different from how Dependabot currently interfaces with other ecosystems. Our goal is to de-duplicate the version-update and security-update logic from Dependabot and have that logic be maintained in the package managers themselves. That way, we can onboard new ecosystems without the additional burden of maintaining versioning and update logic in Dependabot!

While this is currently a work in progress and we don't have a definite timeline, we are aiming to land this sometime in the first quarter of next year.

I hope this answers the questions around Pub/Dart support in Dependabot.
Closing this PR out for now as it has not had an update since May.

@Nishnha Nishnha closed this Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: new-ecosystem Requests for new ecosystems/languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Dart/Flutter languages
10 participants