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 dugite #2

Merged
merged 9 commits into from
Dec 1, 2022
Merged

Conversation

benonymus
Copy link

Description of the Change

In order to support Apple silicon I updated the dugite node_module.
I went as far as possible without breaking changes.

Applicable Issues

Github integration does not work in Apple silicon builds

@benonymus benonymus changed the base branch from master to bump-keytar November 5, 2022 14:21
@mauricioszabo
Copy link

Sorry for the wait - can you bump package-lock.json too? So the tests can run too

@benonymus
Copy link
Author

benonymus commented Nov 10, 2022 via email

@confused-Techie
Copy link
Member

@mauricioszabo @benonymus it looks like the testing is trying to use the old workflow. Maybe I can submit another PR to use our current testing platform and use that on here instead? Just so that everything can actually run properly and test against Pulsar rather than Atom?

@benonymus
Copy link
Author

benonymus commented Nov 11, 2022

I ran into some apple silicon issue with mk-snapshot while I was running npm install. This additional version upgrade aims to fix that. https://github.com/electron/mksnapshot/releases/tag/v11.0.1

@benonymus
Copy link
Author

benonymus commented Nov 11, 2022

@mauricioszabo @benonymus it looks like the testing is trying to use the old workflow. Maybe I can submit another PR to use our current testing platform and use that on here instead? Just so that everything can actually run properly and test against Pulsar rather than Atom?

Feel free to push here!

@benonymus
Copy link
Author

We could go up to latest if we are ok dropping support for node 12: https://github.com/desktop/dugite/releases/tag/v2.0.0

@mauricioszabo
Copy link

@benonymus we're already on Electron 12, that's already Node 14, so I see no problems dropping support for node 12

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Depending on how tests run this looks great to me, and I say lets merge it

@confused-Techie confused-Techie dismissed their stale review November 29, 2022 06:56

Tests reveal this PR is not able to be merged.

@confused-Techie
Copy link
Member

I know this PR is pretty old, but @benonymus would you mind taking a look at getting the lock file in sync? Or with your permission may I work off this branch to see if I can help find a resolution

@benonymus
Copy link
Author

I know this PR is pretty old, but @benonymus would you mind taking a look at getting the lock file in sync? Or with your permission may I work off this branch to see if I can help find a resolution

Hey, I thought I already did that. Or did I miss anything?

@confused-Techie
Copy link
Member

I know this PR is pretty old, but @benonymus would you mind taking a look at getting the lock file in sync? Or with your permission may I work off this branch to see if I can help find a resolution

Hey, I thought I already did that. Or did I miss anything?

Yeah I thought we had covered it to, but I was just looking at the CI results when I commented that originally

@benonymus
Copy link
Author

Oh I just checked the CI, I assumed it is just broken. I will try to take a look at this.

@confused-Techie
Copy link
Member

@benonymus so was just looking at this again today. And sorry seems GitHub wants to keep asking for approval to run workflows on this PR. So I've run them again.

I'm also wondering though, now we do get the tests to build everything properly, but then fail during installation, but I don't think this is because of your PR. This repo is still using the old Atom Action for testing. Which even according to Atom their aren't stable enough to continue running.

So I'm going to hope that you are good here, but will make a PR when I get home today to add the proper testing onto this repo. Then from there you could cherry pick or rebase if you'd like. I'll ping you when I've gotten that up.

But I know I mentioned this before, just getting refamiliarized with our situation over here.

Thanks again for all the effort

@benonymus
Copy link
Author

@benonymus so was just looking at this again today. And sorry seems GitHub wants to keep asking for approval to run workflows on this PR. So I've run them again.

I'm also wondering though, now we do get the tests to build everything properly, but then fail during installation, but I don't think this is because of your PR. This repo is still using the old Atom Action for testing. Which even according to Atom their aren't stable enough to continue running.

So I'm going to hope that you are good here, but will make a PR when I get home today to add the proper testing onto this repo. Then from there you could cherry pick or rebase if you'd like. I'll ping you when I've gotten that up.

But I know I mentioned this before, just getting refamiliarized with our situation over here.

Thanks again for all the effort

Sure, thanks for the update!
Let me know when you are ready!

@confused-Techie
Copy link
Member

@benonymus I've just added the commit to the main branch. If you wanna rebase or cherry-pick it that'd be awesome. Then hopefully that can clear up whatever test weirdness we are getting

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

This change looks good to me. All our tests are passing with the expected results, and hopefully with the changes to dugite we can have better mac performance overall.

@mauricioszabo
Copy link

The failure seems a flaky test, so I say, let's merge it! :)

@mauricioszabo mauricioszabo merged commit 791a45d into pulsar-edit:bump-keytar Dec 1, 2022
@confused-Techie confused-Techie mentioned this pull request Dec 2, 2022
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.

3 participants