doc: add guide on maintaining the build files #16975
Conversation
nodejs-github-bot
added
the
build
There was a problem hiding this comment.
I am not sure if pasting the help text here is useful..although it does give you an idea about what it does if you don't know what it is.
There was a problem hiding this comment.
I think the above paragraph is enough. Including the text here will only drift from the actual help text over time.
There was a problem hiding this comment.
Agreed with @richardlau , don't think we should duplicate the ./configure --help output.
There was a problem hiding this comment.
We should document the similarity/differences between vcbuild.bat and Makefile, though not necessarily in this PR (I am not familiar enough with vcbuild.bat to be comfortable to write a guide on it anyway)
There was a problem hiding this comment.
that'd be quite a job for someone! one of it's biggest problems is that it lacks most of the smarts that make brings to the table and we only do a partial job of emulating the bits that are needed
Makefile
Outdated
There was a problem hiding this comment.
Uh, wait, I don't think this is true..? From what I can tell node-test-binary-arm only runs test-ci-js, But then I am not sure who uses this target (test-ci-native) and how are the addons tested on the pis @nodejs/build
There was a problem hiding this comment.
node-test-binary-arm:
case $RUN_SUBSET in
addons)
PYTHON=python FLAKY_TESTS=$FLAKY_TESTS_MODE make test-ci-native
;;
*)
export CC=should-not-compile CXX=should-not-compile # Prevent building
PYTHON=python FLAKY_TESTS=$FLAKY_TESTS_MODE TEST_CI_ARGS="--run=${RUN_SUBSET},7" make test-ci-js
;;
esac
i.e. it's used for the addons tests on arm-fanned.
There was a problem hiding this comment.
Parallel tests are run in parallel by default (by virtue of -J specified to the test runner in Makefile), even if the make command is -j1.
There was a problem hiding this comment.
There's an awkward catch to this that's not documented anywhere that probably should be since I'm finding Jenkins jobs that don't do this properly: a lot of the -ci targets use this PARALLEL_ARGS variable to insert -j X for tools/test.py (-J isn't used in any of the -ci targets). Unfortunately this variable is only set if $JOBS exists, and this isn't automatically set just because you supply -j X to make. In fact, it's kind of difficult to determine the -j argument inside a Makefile in a cross-platform way (but we should probably try, there's an interesting stackoverflow suggestion here). So you have to give JOBS=X when running the -ci targets.
-J is done automatically for tests on non-ci targets so it takes up "as many cores as you have" for tests as @TimothyGu said.
There was a problem hiding this comment.
@TimothyGu @rvagg Thanks for the correction, I'll add a note in the Makefile about this, and change this part to just "number of threads used to build the binary"
There was a problem hiding this comment.
Typo maintained.
Maybe reword this, e.g. and is maintained to avoid being misread as is not (generated and maintained).
Makefile
Outdated
Makefile
Outdated
There was a problem hiding this comment.
This comment doesn't really convey any additional information than can be inferred from the target name.
Makefile
Outdated
There was a problem hiding this comment.
I think clean is a standard enough makefile target name that this comment shouldn't be necessary. If it is kept, suggest Remove the artifacts... reword.
There was a problem hiding this comment.
This is mostly to distinguish clean from docclean, distclean, test-addons-clean and friends.
There was a problem hiding this comment.
Maybe a note that this target will fail unless NODE_VERSION_IS_RELEASE is #define'd as 1 in src/node_version.h.
There was a problem hiding this comment.
@richardlau good catch, though I think this should be documented in the makefile instead.
There was a problem hiding this comment.
Would a maintainer need to know that? Seems more relevant for users.
Makefile
Outdated
Makefile
Outdated
There was a problem hiding this comment.
maybe use backticks for `make coverage-test`. I read this wrong on first parse and thought it was just awkward wording
Makefile
Outdated
There was a problem hiding this comment.
I think rpi is the only exception, I've just been grepping through jenkins config files and it's the only one I can find.
Makefile
Outdated
There was a problem hiding this comment.
now that I've seen this and noticed CONFIG_FLAGS I've gone and edited node-test-commit-linux-fips to not be an exception here, it now does
PYTHON=python FLAKY_TESTS=$FLAKY_TESTS_MODE CONFIG_FLAGS=--openssl-fips=$WORKSPACE/fipscanisterdir $MAKE run-ci -j $JOBS
so it's using run-ci like the rest. I'll do the same for the new jobs I'm building out in node-test-commit-linux-linked that do a similar ./configure override thing.
So the only thing I'd suggest about this option is that you note that it's called by run-ci, we don't actually call it directly anywhere.
_(FYI I also think that node-test-commit-linux-fips wasn't running tests in parallel due to a mis-application of JOBS and -j and I think that should be fixed now, that would explain why it's been taking more time than it should in some instances).
Makefile
Outdated
There was a problem hiding this comment.
fips isn't an exception here, although it does run test-ci directly a second time with FIPS enabled
coverage is another exception that doesn't run this
Makefile
Outdated
There was a problem hiding this comment.
you could note that these -upload tasks are strictly for release builds on release machines only, unless you think that's obvious
There was a problem hiding this comment.
this should probably be python configure --help
There was a problem hiding this comment.
also worth listing:
BUILDTYPE
JOBS
CONFIG_FLAGS
BUILD_DOWNLOAD_FLAGS
BUILD_INTL_FLAGS
|
are you restricting your comment lines shorter than 80 chars? thanks for doing this btw! I hope we can keep it all in sync as it evolves |
There was a problem hiding this comment.
#vcbuild.bat -> #vcbuildbat ?
There was a problem hiding this comment.
configures and prepares?
There was a problem hiding this comment.
A nit: most of the explanations start with a lower case word, some with an upper case one: Build, Generate, Generate, Enable etc.
There was a problem hiding this comment.
I think these come direct from the configure help, so any changes should be made there.
There was a problem hiding this comment.
@richardlau Yes.
@vsemozhetbyt Do you want to open a PR to fix the configure script? Or I can do that as well.
There was a problem hiding this comment.
Though these are rather simple fixes, I would rather abstain as I know neither Python nor the format of this file. Sorry(
There was a problem hiding this comment.
profileJavaScript -> profile JavaScript ?
nodejs -> Node.js ?
There was a problem hiding this comment.
avilable -> available
There was a problem hiding this comment.
benchamarks -> benchmarks
There was a problem hiding this comment.
docuement -> document
There was a problem hiding this comment.
triggered -> be triggered
Makefile
Outdated
There was a problem hiding this comment.
Remove the and which
If we're going to say this here, is it worth saying something similar for make all?
Makefile
Outdated
Makefile
Outdated
There was a problem hiding this comment.
Hmm I think it's obvious without saying that all the targets here can be run manually (some needs certain environment variables and some probably shouldn't be run manually though)
There was a problem hiding this comment.
I would agree, but if you say:
# Note that this is for developers, it is not run on the CI. See run-ci.
elsewhere, then this line suggests that it's used exclusively by the CI job.
There was a problem hiding this comment.
Agreed with @richardlau , don't think we should duplicate the ./configure --help output.
There was a problem hiding this comment.
I was compiling Alacritty the other day, and I noticed that their make actually defaults to showing help (alias for make help) rather than doing anything. It actually looks really nice, and it'd be really useful if we could do something similar.
I think a help option to make would be less likely to bitrot than separate documentation.
There was a problem hiding this comment.
I think you can just say python configure for both Unix and Windows.
💯 on this, nothing worse than slightly out-of-date documentation that you can never quite trust. |
joyeecheung
force-pushed
the
doc-makefile
branch
from
77be3b8 to
7d77bfe
Compare
joyeecheung
changed the title
|
I think I've addressed most comments. @rvagg @TimothyGu @richardlau @vsemozhetbyt @gibfahn PTAL |
joyeecheung
force-pushed
the
doc-makefile
branch
from
7d77bfe to
e80fb74
Compare
|
Ping @nodejs/build @rvagg @gibfahn @richardlau for reviews, thanks |
joyeecheung
force-pushed
the
doc-makefile
branch
from
e80fb74 to
ce180ee
Compare
|
Rebased. New CI: https://ci.nodejs.org/job/node-test-pull-request/11548/ |
joyeecheung
force-pushed
the
doc-makefile
branch
from
2d3c92a to
fef681b
Compare
|
Rebased. Planning on landing this on Monday if there are no more outstanding reviews. cc @nodejs/collaborators |
PR-URL: #16975 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #16975 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
|
Landed in c91bd2f...89d8657, thanks! |
PR-URL: #16975 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #16975 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #16975 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #16975 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
|
Should this land? It will need a manual backport to 6.x and 8.x |
|
@MylesBorins it's just some doc changes so I would say nope |
11 participants
This documents how to maintain the build files and some commonly-used targets in the Makefile. It also documents about how to update certain targets to reflect the changes on CI (especially the Raspberry Pis and Linux with FIPS).
This is still a WIP, and only talks about the makefile, configure script and the vcbuild.bat script. There are other build files (e.g. the gyp files) that need to be documented later.
Personally I prefer to leave most of the actual documentation in the code, whiling leave some pointers and necessary explanations in the guide so it's easier to get started with maintaining them.
cc @nodejs/build
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
doc, build