wJoenn commented 7 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Package ecosystem

npm

Package manager version

PNPM v9

Language version

Node.js 20

dependabot.yml content

version: 2
updates:
  - package-ecosystem: 'npm'
    directory: '/'
    schedule:
      interval: 'daily'
      time: "15:00"

What you expected to see, versus what you actually saw

Since https://github.com/dependabot/dependabot-core/pull/9668 I assumed updating a package that relies on a 9.0 pnpm.lock.yaml file should just update the given file without issue.

Instead the lockfileVersion is reverted back to 6.0 which rewrites the whole lockfile from scratch

Images of the diff or a link to the PR, issue, or logs

https://github.com/wJoenn/vue-template/pull/237

pboling commented 7 months ago

The logic in the conditional statement is not great, and falls back to a default version of 6. I suggested an improvement which went unnoticed, so perhaps now it will get noticed!

https://github.com/dependabot/dependabot-core/pull/9668#discussion_r1590528791

wJoenn commented 7 months ago

Tbh I don't see how this condition could provoke such bug 🤔

sig { params(pnpm_lock: DependencyFile).returns(Integer) }
def self.pnpm_version_numeric(pnpm_lock)
  if pnpm_lockfile_version(pnpm_lock).to_f >= 9.0
    9
  elsif pnpm_lockfile_version(pnpm_lock).to_f >= 6.0
    8
  elsif pnpm_lockfile_version(pnpm_lock).to_f >= 5.4
    7
  else
    6
  end
end

If pnpm_lockfile_version(pnpm_lock).to_f did end up in the default 6 returned value, the the lockfile version wouldn't be reverted to "6.0" but to an even older version as, as you can see, PNPM's v7 used to use a 5.4 lockfileVersion

In my case because my lockfileVersion ends up at 6.0 it would mean the pnpm version used by Dependabot is v8 for some reason

pboling commented 7 months ago

The existing logic is bizarre. If the sig defines it as returning an Integer, why call .to_f on it? If it starts as an integer it can never result in something like 5.4.

This code has never been mentally processed, merely changed over and over again, and passed a CI.

This is really sad to see.

pboling commented 7 months ago

Additionally, if the parser can't parse a v9 lock file in the first place, then we shouldn't expect it to work. Perhaps it returns nil which is converted to 0.0, which falls through to the 6 default.

If there is no fixture for a v9 lock file in the test suite, then it is impossible that a test written to pass returning v9 is properly testing the code.

wJoenn commented 7 months ago

If it starts as an integer it can never result in something like 5.4.

If we have a look we can see that the method used to get the lockfileVersion is:

def self.pnpm_lockfile_version(pnpm_lock)
  pnpm_lock.content.match(/^lockfileVersion: ['"]?(?<version>[\d.]+)/)[:version]
end

Which results in the proper version float number

irb(main):001> "lockfileVersion: '9.0'".match(/^lockfileVersion: ['"]?(?<version>[\d.]+)/)[:version].to_f
=> 9.0
irb(main):002> "lockfileVersion: '5.4'".match(/^lockfileVersion: ['"]?(?<version>[\d.]+)/)[:version].to_f
=> 5.4

If the sig defines it as returning an Integer, why call .to_f on it?

to_f is not called on the returned value but on a variable inside the method. The method does indeed return an Integer, it being 9, 8, 7 or 6

Additionally, if the parser can't parse a v9 lock file in the first place, then we shouldn't expect it to work. Perhaps it returns nil which is converted to 0.0, which falls through to the 6 default.

First of all, in the same PR that you linked we can see that a test has been added to test parsing a v9 file so there's a fare chance that parsing the file works. The test is not extensive enough that it checks that the version is accurate but there's a good chance ::pnpm_lockfile_version does not return nil.

Even if that was the case it wouldn't be the problem. You're confusing the lockfileVersion and pnpm's version.

When the lockfileVersion is less that 5.4 the the pnpm's version used is 6 But in my case the lockfileVersion was changes to 6.0 which means the pnpm version used by Dependabot was 8

Chill out man You're free to create a PR if you think you have a solution 🙂

fallemand commented 7 months ago

Same issue happening for me: https://github.com/dependabot/dependabot-core/issues/9682

pboling commented 7 months ago

Chill out man You're free to create a PR if you think you have a solution 🙂

I hear you, but...

Dependabot is GitHub, and GitHub is Microsoft. They also own NPM, a competitor project of pnpm.

I'd rather hold Microsoft Github's feet to the fire so they fix their products than do it for them for free.

Microsoft funnels millions to anti-human causes. They claim to have stopped funding election deniers (1, 2), but after they thought scrutiny had passed, they started again (3 (page 5)). They also fund climate-change-deniers in the US, while their AI chatbot preaches conspiracy and election denialism... so the suggestion to help them freely is absurd to me, as is the idea of being "chill" about their "mistakes", and slow response to fix issues which are actively harming their competition (pnpm in this case). Happy to help them help them help themselves though.

Dependabot is the only reason I still use GitHub for any private repos. I'll be happy to discard it if they don't fix this.

tusbar commented 6 months ago

From the rebase logs:

  proxy | 2024/05/10 07:09:48 [009] GET https://registry.npmjs.org:443/pnpm
  proxy | 2024/05/10 07:09:48 [010] GET https://registry.npmjs.org:443/pnpm
  proxy | 2024/05/10 07:09:48 [010] 200 https://registry.npmjs.org:443/pnpm
  proxy | 2024/05/10 07:09:48 [009] 200 https://registry.npmjs.org:443/pnpm
  proxy | 2024/05/10 07:09:48 [012] GET https://registry.npmjs.org:443/pnpm/-/pnpm-9.1.0.tgz
  proxy | 2024/05/10 07:09:48 [012] 200 https://registry.npmjs.org:443/pnpm/-/pnpm-9.1.0.tgz

pnpm v9 seems to be used… though I’m seeing the same behavior: lockfile re-generated to v6.

fallemand commented 6 months ago

From my logs, what's using PNPM 8 is: /home/dependabot/dependabot-updater/repo

updater | Your pnpm version is incompatible with "/home/dependabot/dependabot-updater/repo".
updater | 
updater | Expected version: ^9.0.4
updater | Got: 8.15.6
updater | 
updater | This is happening because the package's manifest has an engines.pnpm field specified.
updater | To fix this issue, install the required pnpm version globally.
updater | 
updater | To install the latest version of pnpm, run "pnpm i -g pnpm".
updater | To check your pnpm version, run "pnpm -v".

tusbar commented 6 months ago

Yes: https://github.com/dependabot/dependabot-core/pull/9687 should fix that.

pboling commented 6 months ago

It still does not use pnpm v9, for me I am getting pnpm v8 still, and additionally the outdated packages are not parsed properly (which I expect is because the format is different from v9).

updater | Dependabot encountered '2' error(s) during execution, please check the logs for more details.
updater | +-------------------------------+
updater | | Dependencies failed to update |
updater | +---------------+---------------+
updater | | drizzle-kit   | unknown_error |
updater | | tsx           | unknown_error |
updater | +---------------+---------------+

The two listed are correctly identified as out of date packages, but it fails to identify the version?

cristianmadularu commented 6 months ago

Same here

pboling commented 6 months ago

pnpm v9 still does not work at all. ✨

Related Issues:

Related PRs

dd-jonas commented 5 months ago

Are there any updates on this? Dependabot has been broken for over a month now

matijs commented 5 months ago

Are there any updates on this? Dependabot has been broken for over a month now

@dd-jonas Would you happen to have a link to a public repo where this is broken for you?

D3strukt0r commented 5 months ago

@matijs this is mine from today (14.06.2024)

https://github.com/D3strukt0r/weleda-webcenter-text-export/pull/53

matijs commented 5 months ago

@D3strukt0r thanks! This is very odd, it's been working flawlessly for me for many weeks now in quite a few different repositories. After a quick look, the main difference seems to be that all pnpm-lock.yamls in the repos I work with are in the root whereas yours is in the ./pwa directory. I have no clue if that makes a difference but I think it shouldn't.

I have Dependabot running on both Actions runners as well as the legacy (?) way of running it.

D3strukt0r commented 5 months ago

@matijs my current setting for this is https://github.com/D3strukt0r/weleda-webcenter-text-export/blob/master/.github/dependabot.yml

  - package-ecosystem: "npm"
    directory: "/pwa"
    target-branch: "develop"
    labels:
      - "dependabot :robot:"
    reviewers:
      - "D3strukt0r"
    schedule:
      interval: "daily"
    groups:
      pwa-dependencies:
        applies-to: version-updates
        patterns:
          - "*"
        update-types:
          - "minor"
          - "patch"

i don't see anything special there. if anybody is interested, it's on a daily schedule so i can see it once it ever gets fixed. in the meantime i'll have to figure out how to get renovatebot running on a server, it's just that i can't quite comprehend their docs yet ... ugh

senkenn commented 5 months ago

@matijs I put pnpm-lock.yaml in the root and got the same error. So it doesn't matter where we put it. https://github.com/senkenn/sqlsurge/actions/runs/9409580347/job/25919775104

...

2024/06/07 00:19:44 [173] 204 /update_jobs/838583963/record_update_job_unknown_error
updater | 2024/06/07 00:19:44 ERROR <job_838583963> Error processing ts-jest (Dependabot::SharedHelpers::HelperSubprocessFailed)
2024/06/07 00:19:44 ERROR <job_838583963>  ERR_PNPM_UNSUPPORTED_ENGINE  Unsupported environment (bad pnpm and/or Node.js version)

Your pnpm version is incompatible with "/home/dependabot/dependabot-updater/repo".

Expected version: ^9.0.6
Got: 8.15.6

This is happening because the package's manifest has an engines.pnpm field specified.
To fix this issue, install the required pnpm version globally.

To install the latest version of pnpm, run "pnpm i -g pnpm".
To check your pnpm version, run "pnpm -v".

pboling commented 5 months ago

It doesn't matter where you put it, in that it is still broken, but it is breaking in a different way. The latest break is the same exact one I see in my private repo. On the latest release of pnpm, 9.3.0, with the lockfile at the root level.

matijs commented 5 months ago

@senkenn (as a workaround,) have you tried adding a "packageManager": "pnpm@9.3.0" field to your package.json? That is another difference between your repository and the repos where Dependabot seems to work flawlessly for me.

(we switched to corepack a while ago hence the packageManager field)

kasperp commented 5 months ago

have you tried adding a "packageManager": "pnpm@9.3.0" field to your package.json?

Adding packageManager to package.json this "solved" the issue for me - thanks @matijs

dd-jonas commented 5 months ago

@matijs I have also tried adding the packageManager field, which seems to work correctly. This is not a solution however as we don't use corepack in our project, and enabling this breaks other parts of the CI. The project is private, but I experimented a bit and I can share the logs here (including the dependabot.yml config): https://github.com/dd-jonas/dependabot-logs/tree/main.

I don't see any errors about the pnpm version being incompatible in either logs though. The biggest difference I can see is that the log without packageManager is much longer. Not sure where it goes wrong 😕

D3strukt0r commented 5 months ago

I agree, adding packageManager also worked for me. Would be great if it also worked without it though. Like, dependabot could certainly read the version, and if it can't it should be taking the latest version, which it isn't

matijs commented 5 months ago

It looks like "packageManager" is a (the?) reason Dependabot + pnpm 9 works for us. I have just done a quick test where I removed the field and forced a Dependabot update and this does result in a downgrade of pnpm-lock.yaml for us as well.

After reverting, adding package.json#engines.pnpm and setting that to ^9 results in a complete failure for me. Somewhere in the Dependabot logs there's the complaint by pnpm about a version mismatch but Dependabot actually went as far as closing the PR because it was no longer able to update the dependency.

bgentry commented 5 months ago

@matijs thank you, this workaround also worked for us! :pray: It's sad that such a workaround is still necessary so long after pnpm v9's release 😞

deivid-rodriguez commented 5 months ago

This should be fixed now, hopefully!

abdulapopoola commented 5 months ago

Thanks @deivid-rodriguez for the fix! Closing this as fixed, please reactivate if the problem still occurs.

dependabot / dependabot-core

PNPM lockfileVersion reverted back from 9.0 to 6.0 #9684

Is there an existing issue for this?

Package ecosystem

Package manager version

Language version

dependabot.yml content

What you expected to see, versus what you actually saw

Images of the diff or a link to the PR, issue, or logs

Related Issues:

Related PRs