Documented basic options, and small cleanups #880
Conversation
Reading the code to get command options isn't a problem for anyone familiar with the tool yet it's a hurdle for beginners. Solution: document the most common options. We can extend this list over time.
Solution: document --loglevel=silent and add --silent shortcut for consistency. (I did not see any existing regression tests for options so it's unclear to me where to add a regression test for this... sorry.)
It's unclear why we'd need an explicit option for this, as it's the default setting. Presumably to allow Release/Debug to be passed to node-gyp. If so, we need to document it. Solution: document this option.
|
The first three commits LGTM. The fourth one seems like a subtle change in behavior: it's possible (but for most people not very useful) to have configurations beside Debug and Release. The last commit is one that, if it can be fixed upstream, it should be. Apropos the commit logs: we use imperative style, e.g. |
|
The last commit should be fixed upstream, and I've reported it, but even if that happens we're stuck with gyp installations all over the place and how long before this change makes it into stable packages... right now the behavior of node-gyp with gyp dependencies is so erratic that IMO node-gyp benefits by protecting its users from this. |
|
Do you have a CONTRIBUTING.md with guidelines for commits and pull requests? That would be helpful. I looked for something, did not find it. |
That's not an issue for node-gyp because we bundle gyp. When the fix lands upstream, I can upgrade the bundled copy or cherry-pick the fix.
Um, I thought we did... but looks like we don't. Filed #881. |
|
Ah, OK, that is neat. I'm not sure I'm ready to start hacking on gyp though. What's your experience with fixes in gyp? If you're pessimistic they'll fix it soon then I'll see what I can do. |
|
FWIW I think --mklocal is useful in any case, as it makes node-gyp work the same way as gyp, with makefiles in the working directory. That makes things simpler on Windows IME. |
They're pretty responsive (they're quicker with reviews than I am with updating the CL) but you'll have to get used to Google's way of doing code reviews (depot_tools, git-cl, rietveld.)
I see you've removed the commit but IIRC, it only touched the configure step, didn't it? The build step will try to invoke |
|
I'd changed both the configure and build steps, and have that change on a branch so if we need to we can still use it. I'm skeptical that gyp upstream will take this problem seriously, so I may come back with a "pretty please," or better argumentation. |
|
Thanks @hintjens. I'm fine with this as it is now, pretty minimal. Would you mind lining up the table separators to make it consistent with the previous table? Re contributing, we've recently been adopting the same style as Node core, most relevant section is here: https:/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit minus the need for a subsystem, so in terms of this PR, you could squash the commits down to a single commit and adopt @bnoordhuis' suggestion for summary it should be good to go. |
|
Re squashing, I think that should probably be 2 commits, one for adding new functionality and one for adding docs. |
|
It might be worth noting that some commands also accept trailing |
|
This is a lot of talk for minor documentation fixes. At this stage, I can't even use node-gyp properly without the --mklocal patch. That means I'm looking to either fork it (and fix it) or write another tool that doesn't depend on gyp. @rvagg with all respect, to ask me to align those columns (obviously I already did, except for the long options as that would make the table unusably wide) and reworking into different sized commits (I've already reworked the commits more than once) is a poor thank you. I'm more than happy to polish the docs and help make node-gyp really shiny, except not like this. Your project doesn't even have written rules, until I asked for them. To enforce unwritten rules and stifle new contributors in discussion over trivial issues is sure way to kill a project. Is there any technical reason you couldn't merge the patches, and then improve them? |
Not a technical reason but a social one: it's easier for us maintainers to get it right from the start than trying to fix things up post facto (if for no other reason that it's probably forgotten about.) I know you work on 0mq and that's a pretty busy project in its own right, but you may underestimate the amount of churn, issues and pull requests we work through each day. Just GH notifications alone are 50 to 200 emails every day (and I do try to keep track of them all.) All that aside, I'm fine with the changes but can you squash and reword the commit log? Cheers! |
|
I appreciate the work you do. Yet this morning I forked node-gyp to node-ninja and will take it from there. I need to improve the native toolchain significantly, bring in a lot of new contributors, and it's not plausible using this process. There are reasons I can do 10-20 commits a day to different projects, while patches flow from other people in all directions. (All the time without things breaking.) My goal is to eventually switch off gyp to ninja, and generate the ninja files directly using the code generation tools (gsl) we're using in ZeroMQ. |
|
@bnoordhuis Could this be merged in? I understand it needs some work, but I'd really like to see this go in. I'm happy to polish it up if necessary. |
|
@gibm Sure, if you're willing to adopt this, just file a new PR with the requested changes. |
Continuation of nodejs#880. Documents options accepted by node-gyp.
Continuation of nodejs#880. Documents options accepted by node-gyp.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. Continuation of nodejs#880.
Documents options accepted by node-gyp. PR-URL: #937 Refs: #880 Reviewed-By: Ben Noordhuis <[email protected]>
Also added --mklocal as a workaround for GYP issue #508.