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

PNPM lockfileVersion reverted back from 9.0 to 6.0 #9684

Open
1 task done
wJoenn opened this issue May 7, 2024 · 26 comments
Open
1 task done

PNPM lockfileVersion reverted back from 9.0 to 6.0 #9684

wJoenn opened this issue May 7, 2024 · 26 comments
Labels
L: git:submodules Git submodules L: javascript T: bug 🐞 Something isn't working

Comments

@wJoenn
Copy link

wJoenn commented May 7, 2024

Is there an existing issue for this?

  • 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 #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

wJoenn/vue-template#237
image

@wJoenn wJoenn added the T: bug 🐞 Something isn't working label May 7, 2024
@pboling
Copy link

pboling commented May 7, 2024

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!

#9668 (comment)

@wJoenn
Copy link
Author

wJoenn commented May 7, 2024

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
Copy link

pboling commented May 8, 2024

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
Copy link

pboling commented May 8, 2024

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
Copy link
Author

wJoenn commented May 8, 2024

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
Copy link

Same issue happening for me: #9682

@pboling
Copy link

pboling commented May 8, 2024

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
Copy link
Contributor

tusbar commented May 10, 2024

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
Copy link

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
Copy link
Contributor

tusbar commented May 13, 2024

Yes: #9687 should fix that.

@pboling
Copy link

pboling commented May 19, 2024

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
Copy link

cristianmadularu commented May 29, 2024

Same here
image

@dd-jonas
Copy link

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

@matijs
Copy link

matijs commented Jun 14, 2024

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
Copy link

@matijs this is mine from today (14.06.2024)

D3strukt0r/weleda-webcenter-text-export#53

image

@matijs
Copy link

matijs commented Jun 14, 2024

@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
Copy link

D3strukt0r commented Jun 14, 2024

@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
Copy link

senkenn commented Jun 14, 2024

@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
Copy link

pboling commented Jun 14, 2024

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
Copy link

matijs commented Jun 16, 2024

@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
Copy link

kasperp commented Jun 17, 2024

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
Copy link

dd-jonas commented Jun 17, 2024

@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
Copy link

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
Copy link

matijs commented Jun 17, 2024

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
Copy link

bgentry commented Jun 17, 2024

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

mertssmnoglu added a commit to mertssmnoglu/mertsismanoglu.com that referenced this issue Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: git:submodules Git submodules L: javascript T: bug 🐞 Something isn't working
Projects
Status: No status
Development

No branches or pull requests