8000 make.bat: Don't override SPHINXOPTS/O from the environment by QuLogic · Pull Request #23634 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

make.bat: Don't override SPHINXOPTS/O from the environment #23634

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 19, 2022
Merged

make.bat: Don't override SPHINXOPTS/O from the environment #23634

merged 3 commits into from
Aug 19, 2022

Conversation

QuLogic
Copy link
Member
@QuLogic QuLogic commented Aug 15, 2022

PR Summary

I have not actually tested this... cc @story645

if defined does require command extensions to be enabled, but a) they're enabled by default, and b) we need them for %~dp0 above already.

Fixes #23622

PR Checklist

Tests and Styling

  • [n/a] Has pytest style unit tests (and pytest passes).
  • [n/a] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [n/a] New features are documented, with examples if plot related.
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).

story645
story645 requested changes Aug 15, 2022
View reviewed changes
@QuLogic @story645
make.bat: Make default SPHINXOPTS match documentation
86f361c 
Co-authored-by: hannah <story645@gmail.com>
@story645
Copy link
Member
story645 commented Aug 15, 2022 •
edited
Loading

your version and this equivalent seem to work and I think this is more direct:

if not defined SPHINXOPTS (
set SPHINXOPTS=-W --keep-going
)

but in both versions SPHINXOPTS= is overwritten.

@QuLogic
Copy link
Member Author
QuLogic commented Aug 15, 2022 •
edited
Loading

I didn't use not defined because that didn't appear to be specified in the docs: https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/if

Does set SPHINXOPTS= set a variable empty, or undefine it in Windows? Are you setting it in the environment or passing it at the end of the make.bat call? (I'm not sure how the latter could work, even though that's what's documented, since I can't see how the rest of the command line is used for anything.)

@story645
Copy link
Member

not is specified as optionally usable with a parameter and I haven't found anything that says it wouldn't work, but also I won't block on that🤷‍♀️

if [not] ERRORLEVEL [else ]

Does set SPHINXOPTS= set a variable empty, or undefine it in Windows?

I think undefines it:

>set SPHINXOPTS=        
>echo %SPHINXOPTS%
%SPHINXOPTS%

Are you setting it in the environment or passing it at the end of the make.bat call?

tried both

story645 added a commit to story645/matplotlib that referenced this pull request Aug 16, 2022
@story645
working windows built docs
dce9582 
edited it so at least the docs are now accurate, hopefully once matplotlib#23634 goes in, either as part of that PR or new one the docs can be amended yet again to add 

> On Windows, either put the arguments at the end of the statement or set the options as environment variables, e.g.:
```bat
set O=-W --keep-going -j4 & make html
```
@story645 story645 mentioned this pull request Aug 16, 2022
Closed
@story645
Copy link
Member
story645 commented Aug 16, 2022 •
edited
Loading

Ok so putting at end doesn't work, put in a quick PR so docs reflect current state of build #23640. Currently testing again and will amend this PR w/ what does work:

>echo %SPHINXOPTS%
%SPHINXOPTS%

>make clean       
Removing everything under 'build'...

>echo %SPHINXOPTS%
-W --keep-going

>set SPHINXOPTS= & set O=-j4 -Dplot_gallery=0 &  make clean
Removing everything under 'build'...

>echo %sphinxopts%
ECHO is on.

>echo %O%
-j4 -Dplot_gallery=0

ETA - sorry committing directly to your branch and feel free to drop, github wouldn't let me create a new branch against your branch ☹️

@story645 story645 self-requested a review August 16, 2022 22:19
story645
story645 approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member
@story645 story645 left a comment

Choose a reason for hiding this comment

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

the .bat file seems to work now

@story645 @QuLogic
DOC: Fix setting make.bat options on Windows
6840108 
figured this made more sense than opening a new PR for the docs
@QuLogic
Copy link
Member Author
QuLogic commented Aug 17, 2022

I don't mind committing here, just not the default commit message.

@QuLogic QuLogic added this to the v3.6.0 milestone Aug 18, 2022
@tacaswell tacaswell merged commit f791518 into matplotlib:main Aug 19, 2022
@QuLogic QuLogic deleted the make-opts branch August 19, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@story645 story645 story645 approved these changes

Assignees
No one assigned
Labels
Documentation OS: Microsoft
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[MNT]: make.bat not parsing sphinxopt
3 participants
@QuLogic @story645 @tacaswell
0