-
Notifications
You must be signed in to change notification settings - Fork 301
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
Support renaming generated files #30
Comments
Great idea. We can make this happen. |
@joneshf in grunt-node-webkit-builder it renames it and also adds the right images. Maybe you can look at that project to inspire your code ;) 👍 |
Thanks. I would like to get node-webkit-builder functioning as the base for grunt-node-webkit-builder as originally intended. I'm just getting started on this so I'll get it done as soon as possible. |
@RobinMalfait thanks, I didn't know about the images either. I'm just renaming it with @gabepaez oh, I had meant this to be a question, more than a request :) But I appreciate you taking it on. I'm more than willing to do the work, just didn't know if it was something this program could/should do. Looking forward to it! |
@joneshf it would be nice if it reads your package.json file to get the appName and stuff also macIcons and winIco :) |
Sorry, I didn't intend on closing this out. I'll look into it now. |
I've got renaming the generated files working on the develop branch 3878c68 but I have only tested on mac. Could either of you verify on other platforms? |
@gabepaez What's the status of this? |
Hi @adam-lynch. This is integrated into the |
@gabepaez thanks. Is there any potential issues because the app isn't named |
@adam-lynch you are absolutely right. Any ideas? |
@adam-lynch @gabepaez If you do allow renaming the executable add a note to the readme. Apps will run fine so long as they don't require any native modules. |
I've been building this way since the npm release and not run into issues so far. Building for linux, osx and win on linux. @4ver what are native modules? |
It's a module that includes code written in c++ which is compiled using node-gyp or nw-gyp. As far as I can remember- the issue only occurs on windows. node-sqlite3 is an example of such a module. |
@adam-lynch According to that repo, it's 100% javascript, so you should be okay. |
@tphalp At the time @adam-lynch commented it did contain a native portion but I have since switched to a windows binary (see: Teamwork/node-afk@a266e46) to avoid having to compile versions for different architectures/nw versions. |
So yeah, not naming the Windows exectuable as We've three options;
cc @gabepaez |
@adam-lynch I think your second idea is the best short term solution. Allowing the user to specify different names on each platform just adds complication to the API to address a bug that only affects a smaller segment of users. Also, doing nothing seems wrong given that the module would be generating broken apps without a built in solution. |
Is the long term solution to fix node webkit so that it doesn't require the executable to be named some arbitrary name? |
That seems reasonable enough... |
I've been out of the loop for a while. Is this still relevant or can the issue be closed? |
Yes and I saw in an NW.js issue recently that they'd like to sort it for NW.js 0.13.0. Not sure what the status of this issue is though tbh. |
Thanks for this program! It has made working with node-webkit a bit easier.
I'd like to suggest that the generated files take on the application's name. In particular:
nw
on linux,node-webkit.app
on osx andnw.exe
on windows. If you're for it, I'll submit a PR, but It's a bit awkward to have this generate the specified names without knowing that's what's going to happen. From the README, it seems that the name supplied (or the one in the package.json) would be the name of the application when it was generated. You could still default it to the original names if not supplied anything. I've not found anything that states this isn't possible, and from testing, renaming seems to work on all os's.Is there any reason why this isn't/can't be supported? If not, expect a PR soon.
The text was updated successfully, but these errors were encountered: