Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 11, 2017

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

@rvagg rvagg added backport-requested-v8.x build Issues and PRs related to build files or the CI. labels May 11, 2017
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label May 11, 2017
@gibfahn gibfahn requested a review from jbergstroem May 11, 2017 10:38
@@ -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 \
Copy link
Member

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 ==

@joaocgreis
Copy link
Member

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.

@joaocgreis
Copy link
Member

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.

@rvagg rvagg force-pushed the replaceme-doc-make-abort branch from d1d1ada to 6aa5896 Compare May 29, 2017 11:53
@rvagg
Copy link
Member Author

rvagg commented Jun 9, 2017

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

Perhaps 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?

@rvagg
Copy link
Member Author

rvagg commented Jun 9, 2017

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

@BridgeAR
Copy link
Member

@rvagg where do we stand here? Should this land or do you want to change some more things?

@BridgeAR BridgeAR added the stalled Issues and PRs that are stalled. label Sep 8, 2017
@BridgeAR
Copy link
Member

@nodejs/build @jasnell @gibfahn is this good to go or should this be closed due to a long inactivity? It could land right away if this is the right thing to do.

@rvagg
Copy link
Member Author

rvagg commented Sep 13, 2017

don't close, I'm on it

@joaocgreis
Copy link
Member

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.

@BridgeAR
Copy link
Member

Ping @rvagg

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

Closing due to long inactivity. @rvagg please reopen if you would like to further pursue this.

@BridgeAR BridgeAR closed this Oct 2, 2017
@rvagg rvagg deleted the replaceme-doc-make-abort branch October 17, 2017 11:03
@rvagg rvagg restored the replaceme-doc-make-abort branch October 17, 2017 11:03
@rvagg rvagg deleted the replaceme-doc-make-abort branch October 17, 2017 11:03
@rvagg rvagg restored the replaceme-doc-make-abort branch October 17, 2017 11:03
rvagg added a commit to rvagg/io.js that referenced this pull request Nov 23, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg added a commit that referenced this pull request Nov 28, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg added a commit that referenced this pull request Nov 28, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg added a commit that referenced this pull request Nov 28, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg added a commit that referenced this pull request Nov 28, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24575
Refs: nodejs#24551
Refs: nodejs#12958
Refs: nodejs#12957
Refs: nodejs#8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg added a commit that referenced this pull request Feb 28, 2019
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
BethGriggs pushed a commit that referenced this pull request Mar 7, 2019
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants