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

fix(index): remove breaking thenify #572

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

NervosaX
Copy link
Contributor

@NervosaX NervosaX commented Jun 1, 2022

Having spent the last couple of hours trying to get a windows builds working in a github action (using wine on a debian machine, node 16), I've been stuck with it failing at some point inside the nw-builder code base.

I've methodically worked my way to finding that the thenify package wrapped around rcedit was causing the function to never resolve here: https://github.com/nwutils/nw-builder/blob/master/lib/index.cjs#L857

A console inside this then showed it was not running, and with a catch I could see there were no errors.

If I however remove the thenify (which shouldn't be necessary, the function is already a promise! https://github.com/electron/node-rcedit/blob/v3.0.1/lib/rcedit.js#L8), the promise resolves correctly.

@ayushmanchhabra
Copy link
Collaborator

ayushmanchhabra commented Jun 1, 2022

Oh weird! I personally haven't run a Windows build via wine. What messages do you get on your console if any? Also would you mind adding a GitHub Action to test the Windows build on this PR (since it looks like you were able to get it to work on your end)?

@NervosaX
Copy link
Contributor Author

NervosaX commented Jun 1, 2022

Unfortunately there were no console message, it just fails to run the rcedit and then "succeeds" with only the text (as in, not a failure exit):

Latest Version: v0.65.0
Using v0.65.0 (sdk)
Create cache folder in /app/cache/0.65.0-sdk
Downloading: https://dl.nwjs.io/v0.65.0/nwjs-sdk-v0.65.0-win-x64.zip

Create release folder in /app/build/NW Builder Test/win64

If I use my branch, I get the following:

Latest Version: v0.65.0
Using v0.65.0 (sdk)
Create cache folder in /app/cache/0.65.0-sdk
Downloading: https://dl.nwjs.io/v0.65.0/nwjs-sdk-v0.65.0-win-x64.zip

Create release folder in /app/build/NW Builder Test/win64
Zipping index.html
Zipping package.json

I've created a branch for you to clone/check on/test locally https://github.com/NervosaX/nw-wine-test

This version doesn't complete
https://github.com/NervosaX/nw-wine-test/runs/6683880730?check_suite_focus=true

This version does complete (the fork I made)
https://github.com/NervosaX/nw-wine-test/runs/6683902974?check_suite_focus=true

How best would this fit in to this project as a test?

@ayushmanchhabra
Copy link
Collaborator

Awesome, thanks! I'll test this later today.

I was able run a windows build on #573. I realise that I didn't use rcedit which is why it succeeded. But that'll be my next step.

@ayushmanchhabra
Copy link
Collaborator

Awesome, thanks! I'll test this later today.

I was able run a windows build on #573. I realise that I didn't use rcedit which is why it succeeded. But that'll be my next step.

I just realised I misunderstood what you meant by rcedit. I was able to reproduce on my Windows 11 (WSL2 Ubuntu) - didn't even need to use docker and wine! Seems like a Windows 64 specific issue because the build step worked for all other platforms.

@ayushmanchhabra ayushmanchhabra merged commit d985bf6 into nwutils:master Jun 2, 2022
@ayushmanchhabra
Copy link
Collaborator

Released in v3.7.1.

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