Skip to content

[SPARK-30880][DOCS] Delete Sphinx Makefile cruft - #27625

Closed
nchammas wants to merge 5 commits into
apache:masterfrom
nchammas:SPARK-30731-makefile-cleanup
Closed

[SPARK-30880][DOCS] Delete Sphinx Makefile cruft#27625
nchammas wants to merge 5 commits into
apache:masterfrom
nchammas:SPARK-30731-makefile-cleanup

Conversation

@nchammas

Copy link
Copy Markdown
Contributor

Split off from #27534.

What changes were proposed in this pull request?

This PR deletes some unused cruft from the Sphinx Makefiles.

Why are the changes needed?

To remove dead code and the associated maintenance burden.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested locally by building the Python docs and reviewing them in my browser.

@nchammas

Copy link
Copy Markdown
ContributorAuthor

I couldn't test the Windows Makefile. Perhaps @DavidToneian or @sarutak could take a look at the changes I made there?

@SparkQA

Copy link
Copy Markdown

Test build #118650 has finished for PR 27625 at commit e75cf35.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member

Comment threadpython/docs/Makefile Outdated
@@ -55,125 +25,3 @@ html:
$(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the only target we actually need? I'm Ok with it if we can't think of any reason to use the other targets. I assume this whole thing was a copy paste.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, html and clean are the only targets we use.

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't reuse the same JIRA ID for different PRs, @nchammas .

@nchammas

Copy link
Copy Markdown
ContributorAuthor

Please don't reuse the same JIRA ID for different PRs, @nchammas .

Responded in two places:

@dongjoon-hyundongjoon-hyun changed the title [SPARK-30731] Delete Sphinx Makefile cruft[SPARK-30880] Delete Sphinx Makefile cruftFeb 19, 2020
@dongjoon-hyundongjoon-hyun changed the title [SPARK-30880] Delete Sphinx Makefile cruft[SPARK-30880][DOCS] Delete Sphinx Makefile cruftFeb 19, 2020
@dongjoon-hyun

Copy link
Copy Markdown
Member

Hi, @HyukjinKwon .
This should not land on branch-2.4.

@DavidToneian

Copy link
Copy Markdown
Contributor

The makefile works for me on Windows. However, if one invokes .\make.bat without arguments, nothing happens. Given that all targets except html and clean have been removed, I find that it is more intuitive to accept .\make.bat (and make on other platforms, for that matter) to mean to build the documentation.

Should you disagree, I propose that .\make.bat / make without arguments should result in a message being printed that tells the user what targets there are.

@nchammas

Copy link
Copy Markdown
ContributorAuthor

I've switched to more minimal Makefiles per @HyukjinKwon's suggestion. make html and make clean look good to me.

Calling make alone gives this:

$ make
Sphinx v2.3.1
Please use `make target' where target is one of
html to make standalone HTML files
dirhtml to make HTML files named index.html in directories
singlehtml to make a single large HTML file
pickle to make pickle files
json to make JSON files
htmlhelp to make HTML files and an HTML help project
qthelp to make HTML files and a qthelp project
devhelp to make HTML files and a Devhelp project
epub to make an epub
latex to make LaTeX files, you can set PAPER=a4 or PAPER=letter
latexpdf to make LaTeX and PDF files (default pdflatex)
latexpdfja to make LaTeX files and run them through platex/dvipdfmx
text to make text files
man to make manual pages
texinfo to make Texinfo files
info to make Texinfo files and run them through makeinfo
gettext to make PO message catalogs
changes to make an overview of all changed/added/deprecated items
xml to make Docutils-native XML files
pseudoxml to make pseudoxml-XML files for display purposes
linkcheck to check all external links for integrity
doctest to run all doctests embedded in the documentation (if enabled)
coverage to run coverage check of the documentation (if enabled)

@DavidToneian - I believe the Windows version should now do the same. Can you confirm?

@SparkQA

Copy link
Copy Markdown

Test build #118688 has finished for PR 27625 at commit f2786c6.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member

retest this please

@HyukjinKwon

Copy link
Copy Markdown
Member

Yeah, let's dont port back to 2.4.

@SparkQA

Copy link
Copy Markdown

Test build #118689 has finished for PR 27625 at commit f2786c6.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #118690 has finished for PR 27625 at commit f2786c6.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member

retest this please

@HyukjinKwon

Copy link
Copy Markdown
Member

@DavidToneian, do you mind if I ask to test the current make.bat out?

@SparkQA

Copy link
Copy Markdown

Test build #118691 has finished for PR 27625 at commit f2786c6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@DavidToneian

Copy link
Copy Markdown
Contributor

The new make2.bat works for me on Windows, but I have two little issues that one might want to fix:

  1. The clean target is not being advertised when running make / .\make.bat. I think it's a useful feature, but the user should be told about its existence.

  2. On Windows, running .\make.bat without arguments prints the help message, which says to run make, when it probably should say .\make.bat. I know this output is due to Sphinx, but maybe one could echo a sentence before to make the user aware.

Thanks!

@HyukjinKwon

Copy link
Copy Markdown
Member

@DavidToneian, thanks for testing it out. These files are actually templates from Sphinx so they might need to fix :D. I am pretty confident about this change because I am already using the exact same file.

@nchammas can you rebase? I think it's good to go.

@SparkQA

Copy link
Copy Markdown

Test build #118748 has finished for PR 27625 at commit 1a0384d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member

Merged to master.

@nchammas
nchammas deleted the SPARK-30731-makefile-cleanup branch February 21, 2020 06:36
@HyukjinKwon

Copy link
Copy Markdown
Member

Just for clarification, I didn't backport this PR because this change is potentially able to make diff in the output.

sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
Split off from apache#27534.
### What changes were proposed in this pull request?
This PR deletes some unused cruft from the Sphinx Makefiles.
### Why are the changes needed?
To remove dead code and the associated maintenance burden.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
Tested locally by building the Python docs and reviewing them in my browser.
Closesapache#27625 from nchammas/SPARK-30731-makefile-cleanup.
Lead-authored-by: Nicholas Chammas <nicholas.chammas@liveramp.com>
Co-authored-by: Nicholas Chammas <nicholas.chammas@gmail.com>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nchammas@SparkQA@HyukjinKwon@dongjoon-hyun@DavidToneian@srowen