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

Bump ppm to a46537c0b7f0eaaef5404ef88003951fdc988c65 #383

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

confused-Techie
Copy link
Member

This PR bumps ppm from 4645ba2905747897b02f56d1a09ca9b3a60a6b8b => a46537c0b7f0eaaef5404ef88003951fdc988c65

Which provides the following PRs:

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 15, 2023

Thank you for including the ppm PRs in this PR description!! It really does help a lot with reviewing, you were very right about that! (Just seeing it now from the other side.)

I confirm the new commit SHA matches the tip of ppm repo... What more else is there to review? I do think I'll wait for the CI to match our expected pass/fail situation, but as soon as that happens this would be an easy approve + merge for me.

EDIT to add: Shouldn't make a difference to the package tests, in theory... But the main editor does call into ppm's files in some situations, and I'd hate for this to have issues identified only after a merge, however unlikely... So, better safe than sorry, but I'm expecting smooth sailing here.

Thanks for doing this so quickly!

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 15, 2023

Unrelated Cirrus failure on the macOS x64 (intel) run. I think we need to brew install python@3.10 instead of just brew install python, since the python alias points to python3.11 as of ~yesterday. (Homebrew/homebrew-core@d6e4341) Python 3.11 had breaking changes that break node-gyp.

Not this PR's fault, though.

As for GitHub Actions: Package tests are in line with the usual, Editor tests are passing.

@confused-Techie
Copy link
Member Author

@DeeDeeG thanks for taking a look at this! I've also noticed the Intel MacOS Build failing recently, but thanks for finding what hopefully is the cause of that, I've already made a PR to apply those changes over on #384 to hopefully get it sorted prior to our release date.

confused-Techie added a commit that referenced this pull request Feb 16, 2023
Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Approved, given that #384 appears to be working, should resolve the (unrelated) Cirrus issue identified here.

And, as mentioned above:

I confirm the new commit SHA matches the tip of ppm repo... What more else is there to review? I do think I'll wait for the CI to match our expected pass/fail situation, but as soon as that happens this would be an easy approve + merge for me.

I can now confirm that the GitHub Actions tests are also where we expect them to be.

Thanks!!

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

Successfully merging this pull request may close these issues.

2 participants