TODO comments in Makefile* and configure

[Trott]

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

[Trott]

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]

I'd love to help 😄

[Fishrock123]

(I've numbered the 4 references in the original comment)

3 Looks like it can't actually be done any time soon.

I know enough about makefile I should be able to help on 1 if anyone wants.

[ojss]

I want to help but not really sure about where to start.

[bnoordhuis]

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 .SECONDEXPANSION but 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]

I will fork/clone the repo and get working on them.
What about 3?

[bnoordhuis]

Seems I'm the author of that comment. I guess it can be removed, replacing optparse.SUPPRESS_HELP with a help message.

[ojss]

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]

.PHONY means 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]

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]

Because it takes time. It's annoying when you're tweaking things and every 1 second change takes 30 seconds to rebuild.

[ojss]

I am looking into .SECONDEXPANSION right now.
Ojas Shirekar

On 27-Jan-2016, at 12:23 AM, Ben Noordhuis notifications@github.com wrote:

Because it takes time. It's annoying when you're tweaking things and every 1 second change takes 30 seconds to rebuild.

—
Reply to this email directly or view it on GitHub #4607 (comment).

[ojss]

Okay, it looks like I am way out of my depth with Makefiles right now and I can’t really find a solution that’s elegant.

So, what’s next? How do I proceed? Should I go with .PHONY or .SECONDEXPANSION?

Ojas Shirekar

On 27-Jan-2016, at 1:21 PM, Ojas Shirekar ojas.shirekar@gmail.com wrote:

I am looking into .SECONDEXPANSION right now.
Ojas Shirekar

On 27-Jan-2016, at 12:23 AM, Ben Noordhuis <notifications@github.com mailto:notifications@github.com> wrote:

Because it takes time. It's annoying when you're tweaking things and every 1 second change takes 30 seconds to rebuild.

—
Reply to this email directly or view it on GitHub #4607 (comment).

[bnoordhuis]

Neither are great solutions but .SECONDEXPANSION is marginally less worse, IMO.

[ojss]

What message should I replace this with:

# 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)

Ojas Shirekar

On 30-Jan-2016, at 12:18 AM, Ben Noordhuis notifications@github.com wrote:

Neither are great solutions but .SECONDEXPANSION is marginally less worse, IMO.

—
Reply to this email directly or view it on GitHub #4607 (comment).

[bnoordhuis]

A help message similar to the other --foo-includes switches.

[ojss]

okay added this:
help="directory containing systemtap header files"

I feel that I won't be able to add anything to the Makefile soon. I need to learn more about Makefiles and that will take quite some time. This is all I can do for now, I cannot take care of 1.

[bnoordhuis]

If you file a pull request, we'll take it from there. Make sure everything from CONTRIBUTING.md is checked off.

[ojss]

I have filed a pull request.
However there are duplicate commits, they appeared after following the suggestion provided by git when it couldn’t push to master.

Ojas Shirekar

On 04-Feb-2016, at 9:44 PM, Ben Noordhuis notifications@github.com wrote:

If you file a pull request, we'll take it from there. Make sure everything from CONTRIBUTING.md is checked off.

—
Reply to this email directly or view it on GitHub #4607 (comment).

[lance]

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]

Checking off the remaining bullet point would be nice but giving it its own issue would lose/scatter the discussion, IMO.

[lance]

@bnoordhuis OK, I took the bait and started to try and get my head around the Makefile and how gyp and node-gyp fit 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 gyp and node-gym are only updated when npm is updated as a whole - right? In that case, would a make prerequisite on the npm directory work OK? If no files were added or removed in the update, this wouldn't actually catch it, however.

[Fishrock123]

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.

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

So, if this is true then gyp and node-gyp are only updated when npm is updated as a whole - right? In that case, would a make prerequisite on the npm directory work OK? If no files were added or removed in the update, this wouldn't actually catch it, however.

Nah, we update tools/gyp separately. I think that is what you are asking?

[bnoordhuis]

In that case, would a make prerequisite on the npm directory work OK?

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/gyp is used for building node itself.

[lance]

@Fishrock123 ok, that link is helpful. Thanks. Are updates to tools/gyp handled similarly?

@bnoordhuis is it typical for anything under deps/npm/node_modules/node-gyp to 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 on deps/npm/node_modules/node-gyp/package.json.

For tools/gyp maybe something similar could be done with tools/gyp/setup.py. This has a version number in it, which I assume would change in the event that gyp was updated.

[bnoordhuis]

Maybe all that's needed is a prerequisite on deps/npm/node_modules/node-gyp/package.json.

I think that could work.

For tools/gyp maybe something similar could be done with tools/gyp/setup.py. This has a version number in it, which I assume would change in the event that gyp was updated.

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.json should be sufficient.

[jasnell]

@Trott ... any reason to keep this open?

[Trott]

@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.