Repository navigation
TODO comments in Makefile* and configure #4607
Description
Ref: #264
There are four TODO comments in the general build scripts for the project. It would be great to either remove them from the code (if they are no longer valid or at least not particularly high value), or get issues opened for them, or just get whatever it is they are addressing addressed. Here they are as of this writing.
- 1: Makefile:
# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
# directly because it calls make recursively. The parent make cannot know
# if the subprocess touched anything so it pessimistically assumes that
# .buildstamp and .docbuildstamp are out of date and need a rebuild.
# Just goes to show that recursive make really is harmful...
# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update.
build-addons: $(NODE_EXE) test/addons/.buildstamp
- 2: Makefile.build:
# TODO(bnoordhuis) Make i18n support configurable.
GYPFLAGS ?= -Dv8_enable_i18n_support=0
- 3: configure:
# TODO document when we've decided on what the tracing API and its options will
# look like
parser.add_option('--systemtap-includes',
action='store',
dest='systemtap_includes',
help=optparse.SUPPRESS_HELP)
- 4: And again in configure:
icu_endianness = sys.byteorder[0]; # TODO(srl295): EBCDIC should be 'e'
/cc @bnoordhuis, @srl295
Activity
Trott commentedon Jan 10, 2016
Whoops, submitted this too soon. I meant to wait another day or so to let any conversation in #264 run its course first. Sorry about that. (I'll leave this open though because closing it just to re-open in 24 hours or so is probably more annoying than just letting it be.)
JungMinu commentedon Jan 10, 2016
I'd love to help 😄
Fishrock123 commentedon Jan 11, 2016
(I've numbered the 4 references in the original comment)
3Looks like it can't actually be done any time soon.I know enough about makefile I should be able to help on
1if anyone wants.ojss commentedon Jan 25, 2016
I want to help but not really sure about where to start.
bnoordhuis commentedon Jan 25, 2016
1 is a real issue but it's hard to fix without manually declaring dependencies on the files in tools/gyp and deps/npm/node_modules/node-gyp. Maybe it's possible to automate with
.SECONDEXPANSIONbut that's too horrible to contemplate.2 can be resolved by removing Makefile.build, I'm pretty sure no one uses it.
4 is a comment that can probably be removed. I don't foresee a time where we support EBCDIC systems.
ojss commentedon Jan 26, 2016
I will fork/clone the repo and get working on them.
What about 3?
bnoordhuis commentedon Jan 26, 2016
Seems I'm the author of that comment. I guess it can be removed, replacing
optparse.SUPPRESS_HELPwith a help message.ojss commentedon Jan 26, 2016
Okay, I removed the comment in configure and removed Makefile.build
Changes that were made can be seen on my fork of the project under branch "issue-4607"
Now for 1, I am not great with Makefiles(would love to be better).
From what I understand we need to force rebuild the system every time gyp or node-gyp gets updated. Am I correct in understanding it?
If yes, iirc we can use .PHONY or .FORCE?
bnoordhuis commentedon Jan 26, 2016
.PHONYmeans make will always rebuild, which is expressly not the intent. TBH, I don't have a good suggestion for fixing 1: it's either a (large) list of static dependencies or a dynamic list +.SECONDEXPANSION. Both solutions are inelegant to say the least.ojss commentedon Jan 26, 2016
I see, both would be quite long..
So leave it as it is?
Also noob question, why don't we rebuild the whole thing? Isn't that better, with a fresh build I mean
bnoordhuis commentedon Jan 26, 2016
Because it takes time. It's annoying when you're tweaking things and every 1 second change takes 30 seconds to rebuild.
24 remaining items
build: add a help message and removed a TODO.
build: remove Makefile.build
build: remove redundant TODO in configure
build: add a help message and removed a TODO.
lance commentedon May 12, 2016
Trolling for places to contribute, I stumbled on this. Seems the only item left remaining is https://github.com/nodejs/node/blob/master/Makefile#L164. Should that instead be an issue on its own and this bigger ball of wax can be closed?
bnoordhuis commentedon May 12, 2016
Checking off the remaining bullet point would be nice but giving it its own issue would lose/scatter the discussion, IMO.
lance commentedon May 13, 2016
@bnoordhuis OK, I took the bait and started to try and get my head around the Makefile and how
gypandnode-gypfit into it all. After about 30 minutes of poking around I think I have some idea of what's going on, but maybe you can confirm for me.It looks like the npm project is copied into the node repository by hand (maybe?) whenever node bumps it's dependency version on npm. I don't see any place where this is automated. But perhaps I just haven't found it yet.
So, if this is true then
gypandnode-gymare only updated when npm is updated as a whole - right? In that case, would a make prerequisite on thenpmdirectory work OK? If no files were added or removed in the update, this wouldn't actually catch it, however.Fishrock123 commentedon May 13, 2016
The npm team does that. Here's the documentation on the process: https://github.com/npm/npm/wiki/CLI-Team-Process#submitting-the-new-latest-x-to-nodejs
Nah, we update
tools/gypseparately. I think that is what you are asking?bnoordhuis commentedon May 13, 2016
That could work iff the prerequisites consisted of all the files in
deps/npm/node_modules/node-gyp. That's not very maintainable unless they're collected dynamically (which may come with gotchas of its own.)By the way, the test add-ons are built with the node-gyp that's bundled with npm.
tools/gypis used for building node itself.lance commentedon May 13, 2016
@Fishrock123 ok, that link is helpful. Thanks. Are updates to
tools/gyphandled similarly?@bnoordhuis is it typical for anything under
deps/npm/node_modules/node-gypto change outside of the context of a PR from NPM? Based on the CLI-Team-Process document, it seems changes would only happen as a result of a PR, and there won't typically be any incremental modifications outside of that in a developer's environment. Maybe all that's needed is a prerequisite ondeps/npm/node_modules/node-gyp/package.json.For
tools/gypmaybe something similar could be done withtools/gyp/setup.py. This has a version number in it, which I assume would change in the event thatgypwas updated.bnoordhuis commentedon May 14, 2016
I think that could work.
Unfortunately, that won't work. GYP doesn't have a release process, the version number is never incremented, and I don't think there is a single file in the GYP tree that's always updated.
That said, for just building the add-ons, a prerequisite on
deps/npm/node_modules/node-gyp/package.jsonshould be sufficient.jasnell commentedon May 30, 2017
@Trott ... any reason to keep this open?
Trott commentedon May 30, 2017
@jasnell I'm OK with closing this and the other TODO/XXX/FIXME issues. If any of those items are things that really ought to be fixed (rather than a wishlist or a "will fix after Magical Feature X is available"), a separate issue should be opened anyway because it's just getting lost in these out-of-date tracking issues.
While I think this issue is superfluous personally, anyone else should feel free to re-open (if GitHub permits them to) or comment requesting this be re-opened.