build: only require docs 'Added' fixes for release - #12958
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
tiniest of style nits, but I think we tend to use = rather than ==
Sorry, something went wrong.
|
This was part of a review by @bnoordhuis in #8325 (comment) . I'd be happy to see this check gone for test builds, I've been floating patches since this was introduced. However, this also affects RCs, and for those this should probably still be in place (cc @nodejs/release). Both use DISTTYPE=custom, so perhaps instead of this add an exception that checks if CUSTOMTAG starts with test. |
Sorry, something went wrong.
|
What about this, to check RCs but not test builds: JaneaSystems@e8573c4 ? @rvagg feel free to take it, discuss, or I can open another PR if you prefer. |
Sorry, something went wrong.
|
@jasnell you've done most of the leading-edge RC builds recently, what do you think about locking in need to do the REPLACEME docs replacements before you get to do an RC? It's either that, or require it only for true release builds. @nodejs/build I've put this into the release Jenkins as a quick workaround for now as I'm messing with these new v8-canary builds and they are impacted too. @joaocgreis it's more than just "test" that impacts it, in fact I've been refactoring the Jenkins parameters and scripts to make even more use of "custom" disttype to do v8-canary as well as test and rc. So IMO we should be exclusive rather than inclusive in the requirements for REPLACEME substitution. if [[ "X${disttype}" != "Xrelease" ]]; then
if [[ "X${disttype}" == "Xcustom" ]]; then
replaceme_version="${CUSTOMTAG}"
else
replaceme_version="${disttype}${datestring}${commit}"
fi
perl -pi -e "s/REPLACEME/${replaceme_version}/g" doc/api/*.md
fiPerhaps we should even consider doing this for non release builds in the Makefile so you don't ever build docs with REPLACEME in them? Unfortunately it'll be inaccurate because you'll see things like "Added in nightly-20170608cgf16cc18a", but close enough perhaps? |
Sorry, something went wrong.
|
Actually I'm just going to go with this in Jenkins for a quick workaround until this is solved: if [[ "X${disttype}" != "Xrelease" ]]; then
perl -pi -e "s/: release-only/:/g" Makefile
fi |
Sorry, something went wrong.
|
@rvagg where do we stand here? Should this land or do you want to change some more things? |
Sorry, something went wrong.
|
don't close, I'm on it |
Sorry, something went wrong.
|
To be clear, if the release team doesn't have an issue with RCs not getting the warning (and @jasnell already approved so I assume that's the case), then I have no objection. |
Sorry, something went wrong.
|
Closing due to long inactivity. @rvagg please reopen if you would like to further pursue this. |
Sorry, something went wrong.
PR-URL: nodejs#24575 Refs: nodejs#24551 Refs: nodejs#12958 Refs: nodejs#12957 Refs: nodejs#8325 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Discovered while trying to do a test build for #12957. There are Added: tags without versions in the docs on master so it's borking on build. Test builds use the DISTTYPE of "custom" which isn't allowed through here, like "nightly" is. So I've inverted the logic since "release" would be the only DISTTYPE (also the default if you don't set one) that's allowed through.
/cc @nodejs/build