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.

Sign up for GitHub

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

Jump to bottom

Locked protobufjs version #12144

Closed
wants to merge 1 commit into from
Closed

Locked protobufjs version #12144

wants to merge 1 commit into from

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Aug 22, 2024

Description

The build command errors on a fresh npm install due to protobufjs/protobuf.js#2022. This locks the version to the latest working version.

Issue number and link

#12143, though I'd like to leave that issue open to track why we have the locked version. We can close it when protobuf addressed the issue.

Testing plan

  1. git clean -dxf
  2. npm install
  3. npm run build

The last command should succeed without error.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code
@github-actions GitHub Actions
Copy link

Thank you for the pull request, @ggetz!

✅ We can confirm we have a CLA on file for you.
jjspace
jjspace reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@jjspace jjspace left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I no longer see the build error in question but I am seeing multiple of these warning blocks that I don't remember seeing before, is this known?

2024-08-22_13-12

Also it seems protobuf itself already has a fix open, do we want to just wait on this a little in case they release a patch really fast before the next release?
Edit: I now see this is causing CI errors across other branches so it might be good to just get it in asap then revert closer to the release

Also have some CI test errors but I don't get those locally, were those known tests that fail sometimes?
@ggetz
Copy link
Contributor Author

ggetz commented Aug 22, 2024

I no longer see the build error in question but I am seeing multiple of these warning blocks that I don't remember seeing before, is this known?

No, please go ahead and open a separate issue. Offhand it could be due to a new version of esbuild.
@ggetz
Copy link
Contributor Author

ggetz commented Aug 22, 2024

Also have some CI test errors but I don't get those locally, were those known tests that fail sometimes?

Yes, documented in #11958.
@ggetz ggetz mentioned this pull request Aug 22, 2024
Merged
6 tasks
@jjspace
Copy link
Contributor

jjspace commented Aug 23, 2024

It seems protobuf was really fast on the new release so I'm going to close this. protobufjs/protobuf.js#2023

I opened #12148 for the esbuild warning.
@jjspace jjspace closed this Aug 23, 2024
@jjspace jjspace deleted the protobufjs-version branch August 23, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority - next release
2 participants
@ggetz @jjspace