bun: remove the native helpers as they appear to be leftover from copy/paste...

[jeffwidman]

I noticed this is all copy/pasted from npm_and_yarn ecosystem...

Can we remove this altogether? A quick grep isn't showing places that we actually call this from the bun code.

Let's see what CI thinks.

[kbukum1]

Hi @jeffwidman,

I think we need helpers. You can see in the following we are using yarn helper for conflict dependencies

• dependabot-core/bun/lib/dependabot/bun/update_checker/conflicting_dependency_resolver.rb

  Line 63 in 0186f3c

  function: "yarn:findConflictingDependencies",

[jeffwidman]

IMO it's a problem if CI isn't catching this... deleting native helpers is a big swathe change, we should at least have a sanity check somewhere that catches if they're completely broken.

What happens if the Dockerfile gets refactored and accidentally breaks building the native helpers? Today that wouldn't be caught...

Anyway, a discussion outside the scope of this specific PR, I'll chat more with the team responsible for core.

[yeikel]

IMO it's a problem if CI isn't catching this... deleting native helpers is a big swathe change, we should at least have a sanity check somewhere that catches if they're completely broken.
What happens if the Dockerfile gets refactored and accidentally breaks building the native helpers? Today that wouldn't be caught...

I can confirm that this is very likely to happen.

For example, I tested removing Node since Bun does not require it and the build currently passes

On a related note, is this actually being used and working as expected?

From the docs:

dependabot-core/bun/lib/dependabot/bun/update_checker/conflicting_dependency_resolver.rb

Lines 34 to 36 in 0186f3c

	# Finds any dependencies in the `yarn.lock` or `package-lock.json` that
	# have a subdependency on the given dependency that does not satisfly
	# the target_version.

But bun does not use yarn.lock or package-lock.json. It supports migrating them
but it uses its own lockfile (I assume that we don't need to support this?)

Is this more left-over code from the copy-paste?

The code suggests that this won't work in bun as arborist is for npm:

dependabot-core/bun/helpers/lib/npm/conflicting-dependency-parser.js

Line 15 in df67efc

async function findConflictingDependencies(directory, depName, targetVersion) {

I am asking because if this is actually used we may need to revert the removal of Yarn (#13404), as the referenced script depends on it. I suppose that the other path would be to refactor this so it does not depend on Yarn.

But, If this is actually used we should be seeing a failures in production about it. Is that something you can check and confirm?

[kbukum1]

IMO it's a problem if CI isn't catching this... deleting native helpers is a big swathe change, we should at least have a sanity check somewhere that catches if they're completely broken.
What happens if the Dockerfile gets refactored and accidentally breaks building the native helpers? Today that wouldn't be caught...

I can confirm that this is very likely to happen.

For example, I tested removing Node since Bun does not require it and the build currently passes

On a related note, is this actually being used and working as expected?

From the docs:

dependabot-core/bun/lib/dependabot/bun/update_checker/conflicting_dependency_resolver.rb

Lines 34 to 36 in 0186f3c

	# Finds any dependencies in the `yarn.lock` or `package-lock.json` that
	# have a subdependency on the given dependency that does not satisfly
	# the target_version.

But bun does not use yarn.lock or package-lock.json. It supports migrating them but it uses its own lockfile (I assume that we don't need to support this?)

Is this more left-over code from the copy-paste?

The code suggests that this won't work in bun as arborist is for npm:

dependabot-core/bun/helpers/lib/npm/conflicting-dependency-parser.js

Line 15 in df67efc

async function findConflictingDependencies(directory, depName, targetVersion) {

I am asking because if this is actually used we may need to revert the removal of Yarn (#13404), as the referenced script depends on it. I suppose that the other path would be to refactor this so it does not depend on Yarn.

But, If this is actually used we should be seeing a failures in production about it. Is that something you can check and confirm?

Thanks @yeikel. In that case we need to largen our cleanup scope before removing helpers.

• Going over the code to find out if any functions from helper is used
• Check if related code are necessary or we can cleanup them.
• Accordingly cleanup helpers.

CC: @jeffwidman

[yeikel]

@jeffwidman I am sure you are busy taking other work given that core is not your focus right now

If you're open, I can try sending a follow up on top of this PR do the cleanup @kbukum1 suggested above

I am asking mostly to avoid duplicate efforts as I don't know if you started on it