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

Enabling multiple make targets and sphinx-build options in Windows make.bat (non-PowerShell) #3741

Closed
bskinn opened this issue May 12, 2017 · 4 comments
Labels
api:cmdline type:enhancement enhance or introduce a new feature

Comments

@bskinn
Copy link
Contributor

bskinn commented May 12, 2017

This may be an obsolete improvement when considered in light of #3145 and #3196, but I've put together modified versions of the make.bat.new_t and ~~~make.bat_t~~~ Jinja templates that enable handling of multiple make targets and (AFAIK) the full suite of sphinx-build command line options.

It runs through all of the arguments passed to make.bat and splits them into %TARGETS% and %OPTIONS%, then re-runs make.bat once for each target, passing in all of the %OPTIONS% that were provided. This makes it somewhat inefficient if, say, -E is passed, but I'm not sure how the behavior could be reasonably implemented otherwise. The script performs how I expect it to in a few minutes of poking at different valid and invalid invocation cases (Windows 7 64-bit, py3.5.1, all other packages those installed by default after a pip install -e . invocation in the repository root). The %OPTIONS% handling accounts (AFAICT properly) for the subset of options that take arguments.

Is this enhancement of interest to the project?

If so, I didn't see anywhere in the test suite where the commandline/make utilities were tested. If I missed something, please advise where to put any tests; or, if I should create a new suite for some tests, please advise of that.

Further: I'm not 100% sure about Vista / XP / 2k compatibility -- would I need to confirm any of those?

Finally: I've made these changes in a branch off of stable for now; please advise if I should rebase to master.

Thanks!

@bskinn bskinn changed the title Enabling multiple make targets and sphinx_build options in Windows make.bat (non-PowerShell) Enabling multiple make targets and sphinx-build options in Windows make.bat (non-PowerShell) May 12, 2017
@tk0miya
Copy link
Member

tk0miya commented May 13, 2017

That's sounds good. All PRs are welcome! I've not used windows for long time, but I'll try to review your PR :-)

If so, I didn't see anywhere in the test suite where the commandline/make utilities were tested.

Yes, there are no testcases for that. Now we're testing only inside python.
And we don't have testing platform for windows. so it is hard to test batch files on CI server.
So please check the improvement manually.

Further: I'm not 100% sure about Vista / XP / 2k compatibility -- would I need to confirm any of those?

It seems all of them are no longer supported even by Microsoft. So I think it is not needed.

Finally: I've made these changes in a branch off of stable for now; please advise if I should rebase to master.

It's tough question. Now we are just going to ship new major version in this weekend.
So stable branch will be replaced by 1.6-release branch after the release.
On the other hand, master branch is used for developing. It will released as v1.7 at a half year (or more) later.
It might be better to go with two steps: 1) send the PR into stable branch at first 2) rebase it on stable HEAD after the release. Then we can merge it for v1.6.1.
How about this?

Note: I know this is an improvement. According to our policy, new features should be merged into master branch. But I feel this might be not a breaking change. So +1 for merge this into stable.

@tk0miya tk0miya added api:cmdline type:enhancement enhance or introduce a new feature labels May 13, 2017
@bskinn
Copy link
Contributor Author

bskinn commented May 14, 2017

@tk0miya This all sounds good to me. I actually have a few more improvements that I want to include now, so I think I'll just wait until the v1.6 release is made, and then merge into stable per your (2).

I agree, that technically it may be more proper to target at master, but it doesn't touch the internals of Sphinx at all; it's a tooling augmentation more than anything, and is accordingly pretty minor. This is why I branched off of stable in the first place -- sounds like my instincts were in the ballpark.

I also have some ideas for a separate script to test the feature -- I'll include that in the PR when I submit it.

bskinn added a commit to bskinn/sphinx that referenced this issue Jun 1, 2017
(COMMIT 1) DEV: Draft Win make.bat multi-argument support

Windows 'make.bat' created by sphinx-quickstart.py now is able
 to handle multiple build targets and also sphinx-build
 command line options
 (http://www.sphinx-doc.org/en/stable/man/sphinx-build.html#options).

Modified make.bat properly handles options that take an value,
 though it REQUIRES there to be a space between the option flag
 and the value passed.

(COMMIT 2) DEV/ADMIN

ADMIN: Add 'feature added' entry for the enhanced make.bat

DEV:

make.bat.new_t: Some new stuff added; not fully done yet

 * Change so that the *last* option character passed determines
   whether an extra argument is to be absorbed from the invocation.
 * Add KEYVALUE env var, in anticipation of implementing the key/value
   detection/handling code
 * Remove some redundant zeroes in env var substringing
 * Fix laggard %HASARG% --> %TAKESARG%
 * Add printing of the make target currently being processed
 * Add exit-on-error behavior on a per-target basis

make.bat_t: Parallel changes as to make.bat.new_t

test_winmake.bat: Batch-script "test suite" for the new make.bat

 * Make '-h' show help
 * Make '-k' not delete the temporary docset when testing done
 * Silent test running fouled by CMD misbehavior; see "Buggy behaviour
   when using CALL" ~2/3 of the way down on https://ss64.com/nt/call.html
 * Creates new Sphinx docset with sphinx-quickstart.py
 * Runs various EXPECT-SUCCESS and EXPECT-FAIL tests
 * Print test summary report at end of execution.

(COMMIT 3) DEV: Test suite

test_winmake.bat:

 * Add a VERBOSE option w/message in help display
 * Refactor testing to subroutine with ~callback to each test run.
   Communication between test setup and test runner is by specified
   variables
 * Change format of pass/fail in test report for better visibility

(COMMIT 4) DEV: Fix bug, restructure output

All in test_winmake.bat

Bug: Add missing '%' in VERBOSE mode 'make' call

Output changes:

 * Define output "headers" for unified printing of test output
 * Add notification for if sandbox docset left in place

(COMMIT 5) REVERT: Restore unmodified make.bat_t

(COMMIT 6) ADMIN: .gitignore and -h display

 * Add tests/winmake/testdir to .gitignore, so that any retained
   test folder is ignored
 * Change test_winmake.bat label/goto structure so that extraneous/
   irrelevant testing-related output is not printed when called
   with '-h'

(COMMIT 7) DEV: Mainly %SILENT%, %ANYFAILED%

make.bat.new_t:

 * Change from %0 to explicit make when making the recursive calls.
   Was some path weirdness causing these calls to fail.
 * Fix %ERRORLEVEL% check by changing to numeric test

test_winmake.bat:

 * Add %SILENT% and handling of '-s' flag; roll out output
   suppression if '-s' supplied
 * Add %ANYFAILED%, as 'sticky' flag for failure at any point in
   the series of tests; add exit based on %ANYFAILED% value
 * Exit with error if both '-v' and '-s' are supplied
 * Add tests:
    * make latex
    * make clean html
    * make html -a
    * make html -E

(COMMIT 8) DEV: Mod for key-value args; add tests

make.bat.new_t:

 * Change -D and -A to be args that take key-value pairs by storing
   %TAKESARG% with a value of 2
 * Implement argument consumption for "%TAKESARG%"=="2" arguments
   that properly passes the args to sphinx-build with the proper
   key=value structure

test_winmake.bat:

 * Add tests:
   * make html -Ec .
   * make clean html -Ec .
   * make clean html epub
   * make clean html epub -E
   * make clean html -D rst_epilog="**THIS**"
 * Update formatting of test summary to handle wider column needed
   for the -D rst_epilog test case

(COMMIT 9) TESTS: More lumped multi-option test cases

test_winmake.bat:

 * Add OK test 'make html -qWE', confirming works ok for lumped
   multi-option arguments without a trailing argument value
 * Add ERROR test 'make html -qW.E', which fails since '-.' is not
   a valid argument to sphinx-build

(COMMIT 10) ADMIN/DEV/DOC

AUTHORS (ADMIN): Add name

doc/invocation.rst (DOC):

 * Minor edits to paragraph introducing environment variables
   used by ``sphinx-build``
 * Added exposition on Linux `make` and Windows `make.bat', including
   description of the new capabilities added to make.bat

templates/quickstart/make.bat.new_t (DEV):

 * Change the exit approach for the (disallowed) `-b` option,
   to use the more proper `exit /b 1` syntax after a new
   `:end_error` label
 * Add comment that the arg parsing needs to be updated if new
   options are added that take values

tests/winmake/test_winmake.bat (ADMIN):

 * Change header in test results to full word "Arguments"
 * Expand info in 'help' display to note that tests don't check
   for whether the build operation completed as intended per the
   arguments passed, but instead just test for a success or
   failure exit ERRORLEVEL.
 * Clarify help as re the `-s` option
bskinn added a commit to bskinn/sphinx that referenced this issue Jun 1, 2017
(COMMIT 1) DEV: Draft Win make.bat multi-argument support

Windows 'make.bat' created by sphinx-quickstart.py now is able
 to handle multiple build targets and also sphinx-build
 command line options
 (http://www.sphinx-doc.org/en/stable/man/sphinx-build.html#options).

Modified make.bat properly handles options that take an value,
 though it REQUIRES there to be a space between the option flag
 and the value passed.

(COMMIT 2) DEV/ADMIN

ADMIN: Add 'feature added' entry for the enhanced make.bat

DEV:

make.bat.new_t: Some new stuff added; not fully done yet

 * Change so that the *last* option character passed determines
   whether an extra argument is to be absorbed from the invocation.
 * Add KEYVALUE env var, in anticipation of implementing the key/value
   detection/handling code
 * Remove some redundant zeroes in env var substringing
 * Fix laggard %HASARG% --> %TAKESARG%
 * Add printing of the make target currently being processed
 * Add exit-on-error behavior on a per-target basis

make.bat_t: Parallel changes as to make.bat.new_t

test_winmake.bat: Batch-script "test suite" for the new make.bat

 * Make '-h' show help
 * Make '-k' not delete the temporary docset when testing done
 * Silent test running fouled by CMD misbehavior; see "Buggy behaviour
   when using CALL" ~2/3 of the way down on https://ss64.com/nt/call.html
 * Creates new Sphinx docset with sphinx-quickstart.py
 * Runs various EXPECT-SUCCESS and EXPECT-FAIL tests
 * Print test summary report at end of execution.

(COMMIT 3) DEV: Test suite

test_winmake.bat:

 * Add a VERBOSE option w/message in help display
 * Refactor testing to subroutine with ~callback to each test run.
   Communication between test setup and test runner is by specified
   variables
 * Change format of pass/fail in test report for better visibility

(COMMIT 4) DEV: Fix bug, restructure output

All in test_winmake.bat

Bug: Add missing '%' in VERBOSE mode 'make' call

Output changes:

 * Define output "headers" for unified printing of test output
 * Add notification for if sandbox docset left in place

(COMMIT 5) REVERT: Restore unmodified make.bat_t

(COMMIT 6) ADMIN: .gitignore and -h display

 * Add tests/winmake/testdir to .gitignore, so that any retained
   test folder is ignored
 * Change test_winmake.bat label/goto structure so that extraneous/
   irrelevant testing-related output is not printed when called
   with '-h'

(COMMIT 7) DEV: Mainly %SILENT%, %ANYFAILED%

make.bat.new_t:

 * Change from %0 to explicit make when making the recursive calls.
   Was some path weirdness causing these calls to fail.
 * Fix %ERRORLEVEL% check by changing to numeric test

test_winmake.bat:

 * Add %SILENT% and handling of '-s' flag; roll out output
   suppression if '-s' supplied
 * Add %ANYFAILED%, as 'sticky' flag for failure at any point in
   the series of tests; add exit based on %ANYFAILED% value
 * Exit with error if both '-v' and '-s' are supplied
 * Add tests:
    * make latex
    * make clean html
    * make html -a
    * make html -E

(COMMIT 8) DEV: Mod for key-value args; add tests

make.bat.new_t:

 * Change -D and -A to be args that take key-value pairs by storing
   %TAKESARG% with a value of 2
 * Implement argument consumption for "%TAKESARG%"=="2" arguments
   that properly passes the args to sphinx-build with the proper
   key=value structure

test_winmake.bat:

 * Add tests:
   * make html -Ec .
   * make clean html -Ec .
   * make clean html epub
   * make clean html epub -E
   * make clean html -D rst_epilog="**THIS**"
 * Update formatting of test summary to handle wider column needed
   for the -D rst_epilog test case

(COMMIT 9) TESTS: More lumped multi-option test cases

test_winmake.bat:

 * Add OK test 'make html -qWE', confirming works ok for lumped
   multi-option arguments without a trailing argument value
 * Add ERROR test 'make html -qW.E', which fails since '-.' is not
   a valid argument to sphinx-build

(COMMIT 10) ADMIN/DEV/DOC

AUTHORS (ADMIN): Add name

doc/invocation.rst (DOC):

 * Minor edits to paragraph introducing environment variables
   used by ``sphinx-build``
 * Added exposition on Linux `make` and Windows `make.bat', including
   description of the new capabilities added to make.bat

templates/quickstart/make.bat.new_t (DEV):

 * Change the exit approach for the (disallowed) `-b` option,
   to use the more proper `exit /b 1` syntax after a new
   `:end_error` label
 * Add comment that the arg parsing needs to be updated if new
   options are added that take values

tests/winmake/test_winmake.bat (ADMIN):

 * Change header in test results to full word "Arguments"
 * Expand info in 'help' display to note that tests don't check
   for whether the build operation completed as intended per the
   arguments passed, but instead just test for a success or
   failure exit ERRORLEVEL.
 * Clarify help as re the `-s` option
 * Add paragraph to help recommending running the tests from
   within a freshly created virtualenv
 * Indicate that this batch file exits with ERRORLEVEL==0 if all
   tests pass; ERRORLEVEL==1 otherwise
@bskinn
Copy link
Contributor Author

bskinn commented Jun 1, 2017

The behavior of the old-style make.bat (in make.bat_t) and new-style make.bat (in make.bat.new_t) were sufficiently different that I abandoned the attempt to make this work for the old-style batch file.

I figure most users are just going to run sphinx-quickstart with the default make.bat options anyways, yielding them the new version, so there's not a lot of value in making it work for both.

@AA-Turner
Copy link
Member

Closing in favour of #5618.

A

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api:cmdline type:enhancement enhance or introduce a new feature
Projects
None yet
Development

No branches or pull requests

3 participants