-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: only require docs 'Added' fixes for release #12958
Conversation
@@ -656,8 +656,7 @@ PACKAGEMAKER ?= /Developer/Applications/Utilities/PackageMaker.app/Contents/MacO | |||
PKGDIR=out/dist-osx | |||
|
|||
release-only: | |||
@if [ "$(DISTTYPE)" != "nightly" ] && [ "$(DISTTYPE)" != "next-nightly" ] && \ | |||
`grep -q REPLACEME doc/api/*.md`; then \ | |||
@if [ "$(DISTTYPE)" == "release" ] && `grep -q REPLACEME doc/api/*.md`; then \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiniest of style nits, but I think we tend to use =
rather than ==
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 |
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. |
d1d1ada
to
6aa5896
Compare
@jasnell you've done most of the leading-edge RC builds recently, what do you think about locking in need to do the @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 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
fi Perhaps we should even consider doing this for non release builds in the Makefile so you don't ever build docs with |
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 |
@rvagg where do we stand here? Should this land or do you want to change some more things? |
don't close, I'm on it |
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. |
Ping @rvagg |
Closing due to long inactivity. @rvagg please reopen if you would like to further pursue this. |
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 areAdded:
tags without versions in the docs onmaster
so it's borking on build. Test builds use theDISTTYPE
of"custom"
which isn't allowed through here, like"nightly"
is. So I've inverted the logic since"release"
would be the onlyDISTTYPE
(also the default if you don't set one) that's allowed through./cc @nodejs/build