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

chore(bld): migrate from rcedit with resedit #1094

Merged
merged 5 commits into from
May 22, 2024

Conversation

CosmoMyzrailGorynych
Copy link
Contributor

@CosmoMyzrailGorynych CosmoMyzrailGorynych commented May 16, 2024

  • Replaces rcedit dependency with resedit

Why?

  • rcedit is quite an ancient package that requires Wine to do anything on Linux/MacOS, and having Wine as a dependency in Linux workers sucks!
  • resedit is instead a vanilla-node.js package that does not require building or external binaries.

Replacing rcedit with resedit allows cross-building Nw.js apps for Windows on mainstream Linux+Node.js container images. (Previously you would make your own to preinstall Wine.) And, of course, it makes nw-builder easier to use on Linux-based personal machines, too!

@ayushmanchhabra
Copy link
Collaborator

Thanks for this. I can see why you'd want to replace rcedit with resedit but doing so would introduce a breaking change to current developers workflow. I'd prefer if resedit methodology can be added alongside rcedit. After getting user feedback on both, we can assess if rcedit should be removed in v5.

@ayushmanchhabra
Copy link
Collaborator

Thanks for this. I can see why you'd want to replace rcedit with resedit but doing so would introduce a breaking change to current developers workflow. I'd prefer if resedit methodology can be added alongside rcedit. After getting user feedback on both, we can assess if rcedit should be removed in v5.

An option such as options.resourceEditor which defaults to rcedit can be added. Then developer has option to use resedit by explicitly setting this option to resedit.

@CosmoMyzrailGorynych
Copy link
Contributor Author

CosmoMyzrailGorynych commented May 19, 2024 via email

@ayushmanchhabra
Copy link
Collaborator

I'd prefer if resedit methodology can be added alongside rcedit.

To toggle between using rcedit and resedit.

@CosmoMyzrailGorynych
Copy link
Contributor Author

Thanks for this. I can see why you'd want to replace rcedit with resedit but doing so would introduce a breaking change to current developers workflow.

Please explain the breaking changes here. There are no API changes and no new dependencies.

@TheJaredWilcurt
Copy link
Member

The resedit package makes no claims of being a drop-in replacement for, or even having API parity with rcedit. It in fact does not even mention rcedit at all. So it would be safe to assume some form of breaking changes would be introduced.

rcedit is quite ancient

Both rcedit and resedit are actively maintained and have had new releases in the past year (November and April).

I agree with Ayush. Offering both as an option makes the most sense. Continue to default to what we have to avoid breaking changes. Re-evaluate when going to the next major in the library.

I'd like to see a more comprehensive comparison of both technologies, what features they each have, if there are any features one has the other doesn't, are there granular controls that differ, etc (other than wine which is the obvious major difference). If we could completely replace rcedit, that would be cool, I am generally in favor of tools that are cross-platform by default and don't require the dev to jump through hoops like setting up wine, or using a separate Windows VM for builds.

@CosmoMyzrailGorynych
Copy link
Contributor Author

The resedit package makes no claims of being a drop-in replacement for, or even having API parity with rcedit.

They don't — my PR handles these differences so the end product is the same. nw-builder uses rcedit to change executable's main icon and change the version info (which includes lots of other keys like company name and copyright), and so does resedit in my PR. Icons are replaced in a correct icon group. List of keys Windows executables can have is neither up to rcedit nor up to resedit — it is constant and thus is the same for both packages.

I use resedit for my nw.js app and for neutralino.js ones, resedit does a great job in both circumstances. Disputes about keeping rcedit seems like an XY problem to me or not understanding the PR.

src/bld.js Show resolved Hide resolved
src/bld.js Outdated Show resolved Hide resolved
src/bld.js Outdated Show resolved Hide resolved
src/bld.js Outdated Show resolved Hide resolved
@ayushmanchhabra
Copy link
Collaborator

Sorry, misunderstood the PR. Went in depth into what resedit does/is capable of. Also went through the PR - no breaking changes.

@ayushmanchhabra ayushmanchhabra changed the title chore(bld): Replace rcedit with resedit chore(bld): migrate from rcedit with resedit May 22, 2024
CosmoMyzrailGorynych and others added 5 commits May 22, 2024 22:03
Co-authored-by: The Jared Wilcurt <[email protected]>
Co-authored-by: The Jared Wilcurt <[email protected]>
Co-authored-by: The Jared Wilcurt <[email protected]>
Co-authored-by: The Jared Wilcurt <[email protected]>
@ayushmanchhabra ayushmanchhabra merged commit 03a55b9 into nwutils:main May 22, 2024
3 checks passed
@CosmoMyzrailGorynych
Copy link
Contributor Author

Thank you for merging in the review changes and the PR — I was busy with my own projects and couldn't add changes in time.

ayushmanchhabra pushed a commit that referenced this pull request Jun 11, 2024
🤖 I have created a release *beep* *boop*
---


##
[4.7.5](v4.7.4...v4.7.5)
(2024-06-11)


### Bug Fixes

* **run:** set stdio behaviour to inherit
([a3d181a](a3d181a))


### Chores

* **bld:** migrate from rcedit with resedit
([#1094](#1094))
([03a55b9](03a55b9))
* **deps:** bump actions/checkout from 4.1.5 to 4.1.6 in
/.github/workflows in the gha group
([#1095](#1095))
([0f1b126](0f1b126))
* **deps:** bump google-github-actions/release-please-action from 4.1.0
to 4.1.1 in /.github/workflows in the gha group
([#1091](#1091))
([316741b](316741b))
* **deps:** bump googleapis/release-please-action from 4.1.1 to 4.1.3 in
/.github/workflows in the gha group
([#1114](#1114))
([e284f5b](e284f5b))
* **deps:** bump the npm group across 1 directory with 3 updates
([#1112](#1112))
([fde3491](fde3491))
* **deps:** bump the npm group across 1 directory with 6 updates
([#1105](#1105))
([eb63ded](eb63ded))
* **deps:** upgrade to eslint v9
([ffe6dd0](ffe6dd0))
* **docs:** add missing platform-specific app options info
([#1093](#1093))
([715097f](715097f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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