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

Update dependencies and support darwin-arm64 (Apple M1) #444

Merged
merged 17 commits into from
Feb 15, 2022

Conversation

pinheadmz
Copy link
Contributor

@pinheadmz pinheadmz commented Jan 18, 2022

This is still a big WIP and a bit of a mess. Our biggest dependencies (webpack/babel/electron) have all gone through several major, breaking updates and so upgrading those packages also requires fixing code.

Goals

  • Enable building from source and running dev mode on Apple M1 macbook pro
  • Reducing security warnings from deprecated packages in npm audit

Notes

  • Node.js supports M1 starting with Node.js v16, I think we should make this the new minimum for developing Bob for this reason
  • We need to fix peerDependencies in hsd-ledger because of a new behavior in Node.js v16. To bypass the error use npm install --legacy-peer-deps

TODOs:

  • update smaller deprecated dependencies like babel-eslint, source-map-url etc
  • update readme and build docs with M1 guide, build for both Apple platforms
  • fix missing UI elements like icons, CSS is not totally right yet
  • fix package build and test: right now, only dev mode is working

Breaking changes addressed in this PR

Webpack 5

Removed Node.js polyfills:

https://webpack.js.org/blog/2020-10-10-webpack-5-release/#automatic-nodejs-polyfills-removed

Browser-compatible modules must be installed separately and there is a plugin for that:

https://github.com/Richienb/node-polyfill-webpack-plugin

--colors is not an option any more:

rails/webpacker#3138

Webpack dev-server

Many config options were migrated, removed, changed:

https://github.com/webpack/webpack-dev-server/blob/master/migration-v4.md

Babel

babel/polyfill is deprecated:

https://babeljs.io/docs/en/babel-polyfill

note: building the package still has issues with the ECMAscript modules, this may be the reason, I haven't tried adding core-js like the deprecation notice suggests

Electron

Apple Silicon support:

https://www.electronjs.org/blog/apple-silicon/

electron.remote deprecated and moved to userland (@electron/remote):

electron/electron#21408

https://github.com/electron/remote

note: in general, electron is moving towards a better separation of main and renderer processes, there still maky be issues

better BrowserWindow options for security:

electron/electron#9920 (comment)

note: like the remote thing I think we should strive to include LESS in the renderer. For example, it would be great not to have to package hsd AT ALL for the UI layer, and rely on ipc calls even for things like validating addresses in the send modal -- does that have to be done in the browser?

Because of the better separation of main and "remote" some things needed to be hacked like the Electron executable path and the Sentry initializer for the renderer process. I could be missing something with my hacks but the errors I was getting looked like it was trying to run Node.js code in the browser... ?

Misc

globalThis replaces global (I'm not sure if this error was coming from webpack or electron):

https://github.com/tc39/proposal-global

@rithvikvibhu
Copy link
Collaborator

rithvikvibhu commented Feb 3, 2022

  • Updated sentry to work with new electron - this was breaking prod builds. In v3, sentry needs to load different libs based on which process it's loaded in: main or renderer. Remember to npm i to update sentry
  • Webpack 5 handles assets better than before so replaced each file type handler with asset/resource.

Tested and (at least) the AppImage works now.

There's 1 issue I'm not sure how it came up now. Opening an auction page of any name errors and doesn't load. Issue is from here:

export function hashName(name) {
assert(verifyName(name));
const slab = NAME_BUFFER;
const written = slab.write(name, 0, slab.length, 'ascii');
assert(name.length === written);
const buf = slab.slice(0, written);
const hash = new SHA3(256);
hash.update(buf);
return hash.digest();
}

hash.update(buf); expects buf to be a Buffer or string, but the isBuffer test inside update() says false for some reason.
Workaround is to replace this line with:

hash.update(name);

Not sure if it's the same hash as writing to buffer and passing it, but this at least loads the page without problems.

@rithvikvibhu
Copy link
Collaborator

Can remove canvas dependency as it's not used in Bob anywhere.

package.json Outdated Show resolved Hide resolved
@pinheadmz
Copy link
Contributor Author

@rithvikvibhu great work on this, thanks for jumping in. I added a commit that fixes the Buffer problem you found simply by replacing the duplicated code in the util file with the original code form hsd 🤷‍♂️

I was able to run Bob in both dev mode and npm run package sucessfully built a working executable for darwin-arm64! 🥳

--> file /Users/matthewzipkin/Desktop/work/bob-wallet/release/mac-arm64/Bob.app/Contents/MacOS/Bob 
/Users/matthewzipkin/Desktop/work/bob-wallet/release/mac-arm64/Bob.app/Contents/MacOS/Bob: Mach-O 64-bit executable arm64

Before I was able to build though I had to manually install dmg-license which seems to be an "optional" dependency of "dmg-builder" which is installed by electorn-builder. We might need to add this package to devDependencies but I wanted to check with you first.

@rithvikvibhu
Copy link
Collaborator

fixes the Buffer problem you found simply by replacing the duplicated code in the util file with the original code form hsd

Lol nice.

I don't have a mac to run and test, will try to set up GitHub Actions tomorrow with macos-latest (x86 though, not M1). Will keep dmg-license in mind.

Of the 4 todo items in your desc, 3 and 4 are done, Can't do 2, you'll have a much better picture. Can do 1 and clean up unused deps (like canvas). With a final check on all platforms, think we can merge and move on to other issues without worrying about conflicts with this one. Anything else that's left?

@pinheadmz
Copy link
Contributor Author

electron-userland/electron-builder#6573

"It's required for Mac builds, but it needs to remain optional in order for electron-builder to be installable on windows/linux."

@pinheadmz
Copy link
Contributor Author

@rithvikvibhu I added notes to the README and updated the package.json scripts. I test both dev mode and packaged builds on both Intel and arm64 Macs running nodejs 16, and even did tests with Ledger in all those environments and everything worked! I can test Ubuntu and Windows as well later today.

One thing that theoretically should work but isn't super clean yet is building for Intel mac with an arm64 mac. I tried a few different things and always ended up with some kind of error when launching the build. I left those directions in the README anyway and maybe we can revisit that in the future as well. Unfortunately for now this means that we need two different Mac machines to build the arm64 and intel binaries for release.

@pinheadmz pinheadmz marked this pull request as ready for review February 9, 2022 17:48
@pinheadmz
Copy link
Contributor Author

✅ tested on Ubuntu 20 dev mode / appimage + ledger

@pinheadmz
Copy link
Contributor Author

✅ tested on windows 10 dev mode and production build, nodejs v16 with ledger

@pinheadmz
Copy link
Contributor Author

pinheadmz commented Feb 9, 2022

Rebased on master - we should be good to go! Could probably use more diverse testing in terms of actions like shakedex and watchlist, but basic functions all seem good.

@lukeburns
Copy link
Contributor

lukeburns commented Feb 9, 2022

Building on darwin arm64 node 16 for me, but not node 17 — seems to be related to nodejs/node-gyp#2534

@pinheadmz
Copy link
Contributor Author

@lukeburns thanks, I think odd-numbered nodejs versions are generally considered unstable? I usually stick to LTS which right now is:

Latest LTS Version: 16.14.0 (includes npm 8.3.1)

app/sentry.js Outdated Show resolved Hide resolved
@rithvikvibhu
Copy link
Collaborator

rithvikvibhu commented Feb 10, 2022

Sorry, should've committed before your testing. But these are minor updates to deps and if it works on one system, it should work everywhere - dev and package both work for me on linux.

22 vulnerabilities (15 moderate, 7 high)

is down to

5 vulnerabilities (2 moderate, 3 high)

And npm outdated list is down from 50 to 19.

Windows,16 ci isn't passing in this PR, but same code does on my fork ¯\(ツ) GH caching something, force-pushing with no changes made it work.

And one last Q about Sentry above.

@chikeichan chikeichan merged commit 6f3f93b into kyokan:master Feb 15, 2022
@pinheadmz pinheadmz deleted the update-deps branch July 1, 2022 15:13
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.

4 participants