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

When is requirements_to_unlock: :all used? #600

Closed
omnibs opened this issue Jul 25, 2018 · 10 comments
Closed

When is requirements_to_unlock: :all used? #600

omnibs opened this issue Jul 25, 2018 · 10 comments

Comments

@omnibs
Copy link
Contributor

omnibs commented Jul 25, 2018

Just did a search for requirements_to_unlock in the codebase and only found :all being passed in maven tests. It's also not mentioned in the dependabot-script.

I'm also confused with the whole flow between :own, :none and :all.

:none seems to be used only in can_update? and only when the dependency doesn't appears_in_a_lockfile?. It doesn't propagate it outwards, so I don't understand how we can arrive at updated_dependencies with requirements_to_unlock: :none.

On :all, I have no ideas. I imagine Dependabot itself decides to send it in for updated_dependencies but I can't infer the when/why.

@greysteil
Copy link
Contributor

Hey @omnibs,

This is something of an implementation detail, but here's the thinking on requirements_to_update.

  • It's there to determine what the latest version you can update to in the context of all your other dependency versions is. For languages that have a resolver with the concept of version conflicts that won't always be the latest version.
  • It therefore only really applies to Ruby, PHP, Go, Elixir and Python (with pipenv / pip-tools). Other resolvers don't have version conflicts
  • The three options works as follows:
    • none means that none of the requirements specified in the manifest file are allowed to change. For a language that has lockfiles there's still a chance that Dependabot will generate a PR in that case (because it is still allowed to update the lockfile). For languages without a lockfile Dependabot doesn't have any work to do in that case (since no files will change)
    • own means the only requirement in the manifest which is allowed to change is the requirement for this specific dependency. All other manifest requirements have to stay the same. This is how Dependabot does most of its updates
    • all means that any requirement in the manifest can be changed in order to try to update the dependency in question. This is only implemented for Ruby and Java, because getting the logic right on it is hard. In Java it manifests as PRs which update a property in the POM, and by doing so update more than one dependency at once. Those property updates wouldn't be allowed with own, because they implicitly unlock more than one dependency.

Hopefully that helps clarify?

@omnibs
Copy link
Contributor Author

omnibs commented Jul 25, 2018

It helps a bit. I'm thinking I might need :all, but I'll try to talk through my scenario, if you have time I'd love advice:

I'm working on Elm support, and Elm has an elm-package.json and an exact-dependencies.json. The latter sounds like a lock-file, but isn't. It reports what elm-package install actually did and is not supposed to be kept in version control.

The elm-package.json file doesn't always reflect reality. That's why we have exact-packages.json.

We might say, for instance:

  "dependencies": {
    "rtfeldman/elm-css": "13.0.0 <= v <= 13.0.0",
    "NoRedInk/datetimepicker": "3.0.1 <= v <= 3.0.1"
  }

NoRedInk/datetimepicker depends on rtfeldman/elm-css. On version 3.0.1 it allowed up-to elm-css 13.0.0, and on 3.0.2 it allows up-to 14.0.0.

If Dependabot updates datetimepicker to 3.0.2, elm-package install will actually go ahead and install elm-css 14 for us, without touching the elm-package.json file, only affecting exact-dependencies.json. There's no way to prevent that from happening.

So it's kinda like elm-package operates in :all mode all the time. It just doesn't reflect things on elm-package.json when it "unlocks" other dependencies.

I'm thinking I could either:

a) Detect when bumping a dependency affects other dependencies in exact-dependencies.json and not report that as last_resolvable_version and expect Dependabot to call me with :all later
b) Ignore :none, :all and :own and always work under the assumption I'm in :all:

  • latest_resolvable_version is always the highest elm-package install is able to resolve
  • updated_requirements always brings together updates to other dependencies if we find they've been updated by running elm-package install and digging into exact-dependencies.json. This will make Dependabot keep elm-package.json always consistent with exact-package.json.

Which do you think makes more sense?

Would b) work? I'm leaning towards it.

If a) is better, how do I communicate to Dependabot it can call me with :all?

@omnibs
Copy link
Contributor Author

omnibs commented Jul 25, 2018

Investigating some more, I see b) won't work. If I don't receive :all I'll only be able to update 1 dependency (here or here).

I'll change to a simpler question:

How do I make sure my UpdateChecker receives updated_dependencies with requirements_to_unlock: :all?

I'm imagining there must be some logic more complex than what's in dependabot-script/update-script.rb:89-90 that falls back to :all and I'd like to ensure my code works with that.

@greysteil
Copy link
Contributor

Hey, sorry for the slow reply, was crunching on getting Go support out (now done 🎉).

Elm support would be awesome! Thanks for looking at it!

In the main Dependabot updater script, we have the following:

def requirements_to_unlock(checker)
  if job.lockfile_only? || !checker.requirements_unlocked_or_can_be?
    if checker.can_update?(requirements_to_unlock: :none) then :none
    else :update_not_possible
    end
  elsif checker.can_update?(requirements_to_unlock: :own) then :own
  elsif checker.can_update?(requirements_to_unlock: :all) then :all
  else :update_not_possible
  end
end

(The job.lockfile_only? comes from our database for whether this user wants their manifest updating - you can ignore that bit.)

As a result, if you proceed with the approach suggested in (a) that should work perfectly.

That's very surprising behaviour from elm-package install! However, am I right in thinking that with Elm you can have multiple versions of the same dependency (as with npm and yarn)? That would make that behaviour make a little more sense, I guess (assuming it kept a version at the expected version at the top level.

The other important consideration when adding a new language is that, if at all possible, Dependabot shouldn't actually do installs. With npm for example there's a --package-lock-only option that we tap into, and with Bundler we hack around with the internals of the package manager. Doing so allows us to keep Dependabot fast.

@omnibs
Copy link
Contributor Author

omnibs commented Jul 25, 2018

Hey, sorry for the slow reply

No way, this is super fast =D

That's very surprising behaviour from elm-package install!

I know rite?!?

However, am I right in thinking that with Elm you can have multiple versions of the same dependency (as with npm and yarn)?

It doesn't support multiple versions of the same dependency. I don't really know why elm-package behaves this way. Evan errs on the side of lacking features unless he has comprehensive use-case study to back them, this might be one case of that.

In the main Dependabot updater script, we have the following:

This is super helpful! 😻

if at all possible, Dependabot shouldn't actually do installs

It's a little complicated with Elm. There's nothing like --package-lock-only, there's no documented public API or CLI tool to help with dependency resolution other than running elm-package install itself, and it's indeed kinda slow (I think it fetches dependencies sequentially).

Let me know if it's sensible to move forward with Elm support given the above.

One hacky alternative would be forking elm-package, but I'd rather wait for Elm 0.19 which should be somewhere around the next 3 months than to do it on the current one.

@greysteil
Copy link
Contributor

greysteil commented Jul 25, 2018

Let me know if it's sensible to move forward with Elm support given the above.

One hacky alternative would be forking elm-package, but I'd rather wait for Elm 0.19 which should be somewhere around the next 3 months than to do it on the current one.

Forking elm-package sounds really grizzly - we definitely don't want to do that! Have you looked into any way to hack into its innards? Take a look at what we do for yarn and composer for example. It can get pretty ugly, mind...

Edit: The ease of hacking around in the above way really depends on how easy / sane it is to monkey patch in a language, and I have no idea about Elm. If shelling out to an Elm script would allow us to import the bits of the package manager that we need, or to patch over the bits that actually do the install, then it's a great move. If the install is intricately entwined with resolution then it's less good.

How slow are we talking for an install on a real life project?

@omnibs
Copy link
Contributor Author

omnibs commented Jul 26, 2018

We have one of the largest Elm codebases in the world and our deps take 50s to download from my local machine in Brazil (130ms ping to http://package.elm-lang.org/). I'm imagining that's too much.

The package manager is a Haskell binary and the code for dependency resolution is just in there, I can't see a way to monkey patch that :<

I talked to some colleagues who came up with solutions:
a) Parsing yes n | elm-package install output: Elm shows a plaintext preview of what it's about to do and asks for confirmation, that's super fast (800ms for the same deps above)
b) Porting out the dependency solver from elm-package. It's open source and one colleague who's the maintainer of ellie-app.com has ported the code to other languages at least 3 times before.

I'm opting to go with a) bc we can write tests for real known scenarios (like the elm-css vs datetimepicker above) and know when our expectations deviated from elm-package install behavior. If we reverse engineer we could either introduce bugs or deviate from spec and be unaware.

@greysteil
Copy link
Contributor

Yep, 50s is too long for Dependabot to spend at the resolution stage (because it has to do it many times over).

Option (a) sounds good to me - sounds like that does exactly what we want, which is curtail elm-package after resolution but before install.

@hmarr - any thoughts?

@omnibs
Copy link
Contributor Author

omnibs commented Jul 26, 2018

Another thing about Elm is the release cycle is very long. Elm-package hasn't changed since Nov 2016.

The next version (Elm 0.19) for instance will introduce a new dependency file format, so we don't expect this work to break for anyone ever. We expect to have to re-do this work for 0.19 and probably keep support for both 0.18 and 0.19 in parallel in Dependabot.

@greysteil
Copy link
Contributor

Cool - supporting both versions once 0.19 comes out sounds sensible to me. Super excited about this!

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

No branches or pull requests

2 participants