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: Replace spaces in package directory name with a hyphen #3387

Closed
wants to merge 1 commit into from

Conversation

felixrieseberg
Copy link
Member

If your app's name contains a space, the temporary package directory will also contain a space. That works fine for everything Electron and Node, but if you have a native module that uses the classic gyp/make combo, paths with spaces are not allowed and things will break. An earlier filenamify does not replace spaces because spaces are generally legal, they just break (some) native module builds.

With this PR, spaces in your app name in packageDir will be replaced with a -, just the same we replace other special characters.

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes: fix: Don't create package directories with spaces

@felixrieseberg felixrieseberg requested a review from a team as a code owner October 23, 2023 18:07
Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM

@felixrieseberg
Copy link
Member Author

One thing that's really important here: This won't work without also changing electron-packager, which would be a breaking change.

I'm taking a look at that, too.

@erikian
Copy link
Member

erikian commented Oct 25, 2023

@felixrieseberg I'm adding the breaking-change label so we can keep track of those — feel free to remove it if it's not the case!

@felixrieseberg
Copy link
Member Author

Totally fair, but great news: We can fix this here (electron/packager#1585) without breaking forge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants