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.

Sign up for GitHub

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

Jump to bottom

Question about is_win32 flag #79

Closed
kfranqueiro opened this issue Jun 20, 2015 · 3 comments
Closed

Question about is_win32 flag #79

kfranqueiro opened this issue Jun 20, 2015 · 3 comments
Labels
question ❓ Question about using Electron Packager. Not supported in this issue tracker.

Comments

@kfranqueiro
Copy link
Contributor

I've been digging through the code while working on my all branch, and noticed this block in common.js.

I wouldn't necessarily bat an eye at this in itself, but what strikes me as odd is that it's only set to true specifically in the win32 module. I would think that if anything, this needs to be set to true when the host platform (i.e. os.platform()) is win32, not the target platform.

Can anyone corroborate this?
@malept malept added the question ❓ Question about using Electron Packager. Not supported in this issue tracker. label Jun 21, 2015
@malept
Copy link
Member

malept commented Jun 21, 2015

I would think that if anything, this needs to be set to true when the host platform (i.e. os.platform()) is win32, not the target platform.

Perhaps. It was originally only added to win32.js (via #46), so when I refactored, I didn't want to change that behavior.

@kfranqueiro
Copy link
Contributor Author

I've been manually testing everything I can think of and ignore is one of the last things left on my list, so hopefully I'll hit that sometime in the coming week to confirm this.

@kfranqueiro
Copy link
Contributor Author

I've confirmed tonight that this is indeed wrong - for example, --ignore='ignore/this' will ignore correctly for windows builds on windows, but not linux/mac builds on windows. The check should be detecting against the host platform, not the platform being targeted. (Or perhaps look at path.sep and if it's not already /, replace based on it.)

I should be able to come up with a PR for this.

kfranqueiro referenced this issue in kfranqueiro/electron-packager Jun 23, 2015
@kfranqueiro
Fix path separator replacement for ignore on Windows
63db115 
This needs to be done when Windows is the host OS regardless of
the target platform, not vice versa.

Fixes #79.
@kfranqueiro kfranqueiro mentioned this issue Jun 23, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question ❓ Question about using Electron Packager. Not supported in this issue tracker.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@malept @kfranqueiro