-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
[3.5] bpo-31036: use an existing Misc/NEWS rather than trying to use blurb #2874
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
Conversation
Doc/Makefile
Outdated
# (building from a newer repo) and use blurb to generate the NEWS file. | ||
@if [ -f ../Misc/NEWS ] ; then \ | ||
echo "Using existing Misc/NEWS file"; \ | ||
cp ../Misc/NEWS build/NEWS; \ |
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.
Would it be worth it to use $INSTALL_DATA rather than cp?
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.
I don't think so. $INSTALL_DATA is not currently used in the Doc Makefile and we're not installing anything. The cp is just for a temporary build file.
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.
Assuming it works ;-) LGTM
echo "Building NEWS from Misc/NEWS.d with blurb"; \ | ||
$(BLURB) merge -f build/NEWS; \ | ||
else \ | ||
echo "Neither Misc/NEWS.d nor Misc/NEWS found; cannot build docs"; \ |
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.
Actually, as I discovered several times (inadvertently), the docs will build without a valid NEWS file. whatsnew/changelog.html will be empty but otherwise the build is unharmed. I agree that we should probably fail noisily here, I merely offer as an alternative that we could complain noisily but allow the build to continue.
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.
Yeah, we could make that non-fatal. But one or the other (Misc/NEWS or Misc/NEWS.d should be there, I'd think. And the previous Docs build did assume that Misc/NEWS was in the parent directory so that's not new; it might be nice to allow for out of tree builds but that's a separate feature.
Would you consider backporting to 3.4? |
Doc/Makefile
Outdated
@@ -40,7 +40,19 @@ help: | |||
|
|||
build: | |||
-mkdir -p build | |||
$(BLURB) merge -f build/NEWS | |||
# Look first for a Misc/NEWS file (building from a source release tarball | |||
# or old repo) and use that, otherwise look for a Misc/NEWS.d directory |
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.
Oops, those comments need to be dedented otherwise they seem to echo.
Sure I can backport to 3.4 once we agree this is the right approach. I guess I'd like to see @Doko's comments, with the understanding that this only fixes one of his stated use cases. |
It seems @doko42 is too busy to work with us on this. I say we merge this and hope for the best. Please backport to 3.4! |
…blurb (python#2874) (cherry picked from commit 3de1448)
This change should allow docs to be built from either a Git repo (with Misc/NEWS.d files) or from a source tarball (or other source) that has a Misc/NEWS file (as generated by blurb or otherwise). The PR is against 3.5 since blurb support has not yet been pushed into 3.6 and master.
https://bugs.python.org/issue31036