search

nwutils / nw-builder

Build NW.js applications for Linux, MacOS and Windows
MIT License
1.68k stars 302 forks source link

`build` changes cwd #1186

Closed breavyn closed 3 months ago

breavyn commented 3 months ago

Issue Type

Current/Missing Behaviour

Calling nwbuild with mode: "build" and manageManifest will change the processes cwd to use the package manager. This requires the caller to save and restore the cwd when using nwbuild.

https://github.com/nwutils/nw-builder/blob/8f4e07dfbda348fcc832694d346be7616bfb1f4b/src/bld.js#L233

A similar issue exists here also. https://github.com/nwutils/nw-builder/blob/8f4e07dfbda348fcc832694d346be7616bfb1f4b/src/bld.js#L421

Expected/Proposed Behaviour

The package manager should be executed with the needed cwd, without changing the calling processes cwd.

I can submit a PR if desired.

ayushmanchhabra commented 3 months ago

@breavyn PR would be great. But before that would like to understand how this is a bug? How does saving and restoring the cwd lead to buggy behaviour with respect to the built application?

breavyn commented 3 months ago

The nwjs application itself is built correctly. However, silently changing the cwd is hostile to users trying to call nwbuild from a script. It causes any further filesystem related operations to have unintended outcomes.

I propose either not changing the cwd in the first place, which is a trivial change. Or clearly documenting that this is intended behaviour, suggesting the user save and restore the cwd themselves if needed.

const cwd = process.cwd();
await nwbuild({...});
process.chdir(cwd);

Keeping in mind changing this behaviour would break scripts relying on the cwd change.

ayushmanchhabra commented 3 months ago

@breavyn I understand now, please go ahead with the pull request.

Keeping in mind changing this behaviour would break scripts relying on the cwd change.

I think as long as the application builds correctly in managed manifest mode, it should be fine.