Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Build in a virtual environment on Windows (#267) #268
Conversation
Add to the Windows build script a clause to create and use a virtual environment and install Sphinx there, comparable with the Unix Makefile. The user may suppress this behaviour by defining a her preferred command in the environment variable SPHINXBUILD, as before.
| > .\make html | ||
| in the checkout directory, and again this will write the files to the | ||
| ``_build\html`` directory. |
ezio-melotti
Sep 28, 2017
Member
In order to avoid repetitions and keep the instructions concise, I think it would be better to say:
To build the devguide on an Unix-like system use:
$ make html
On Windows, use:
> .\make html
You will find the generate files in _build/html.
zware
Sep 28, 2017
Member
The .\ shouldn't even be necessary unless that's a PowerShell-ism; the same command should work on both platforms.
| .. code-block:: ps1con | ||
| PS> Set-ExecutionPolicy -Scope CurrentUser RemoteSigned | ||
ezio-melotti
Sep 28, 2017
Member
I'm not on Windows so I'm not sure how prevalent the use of PowerShell is but this seems a bit specific for the devguide.
Do you think PowerShell users can easily figure out the problem and the solution if they try to run that command from PS and get an error? If so, this should probably be removed; if instead this is not particularly obvious then it can stay.
jeff5
Sep 28, 2017
Author
Contributor
Ok.
In the "Creator's Update" to Windows 10, a few months back, Microsoft took away convenient access to cmd from the File explorer (which is how I'd normally start my day), leaving only PS. I took the hint and rewrote my scripts. It is better IMO and should be considered the new default. I can't speak for uptake either, but yes, people have probably figured out the permissions thing.
zware
Sep 28, 2017
Member
I agree that this probably doesn't need to be here.
By the way, right click the start button for an easy shortcut to Command Prompt (in regular and admin flavors), unless they also removed that in which case I'm never updating my VM again :)
|
|
||
| if "%1" == "clean" ( | ||
| for /d %%i in (%BUILDDIR%\*) do rmdir /q /s %%i | ||
| del /q /s %BUILDDIR%\* | ||
| goto end | ||
| ) | ||
|
|
||
| rem If we get this far, we're going to use the SPHINXBUILD command |
ezio-melotti
Sep 28, 2017
Member
Unless we got a "clean" "check", "serve", "help", or "" we should get here, right?
jeff5
Sep 28, 2017
Author
Contributor
Yes. The venv bit is here rather than up top for that reason. Perhaps it was someone's logic to order the targets so in anticipation. Happy to assume so.
| @@ -183,3 +196,4 @@ cmd /C %PYTHON% tools\serve.py %BUILDDIR%\html | |||
| goto end | |||
|
|
|||
| :end | |||
| endlocal | |||
| rem If we get this far, we're going to use the SPHINXBUILD command | ||
| if not defined SPHINXBUILD ( | ||
| rem If it is not defined, we build in a virtual environment | ||
| if not exist venv ( |
zware
Sep 28, 2017
Member
This needs some protection against running the script from somewhere other than the dir holding the script. I think if not exist %~dp0venv should do it, but make sure I remembered the correct flags.
jeff5
Sep 28, 2017
Author
Contributor
Sorry, missed this comment the first look. I'll think about this. First thought is that it didn't have it before.
Second is that it might be useful and another thing I could take from the CPython docs build. I guess it would pushd to the right place ... https://github.com/python/cpython/blob/8d59aca4a953b097a9b02b0ecafef840e4ac5855/Doc/make.bat#L4
jeff5
Sep 28, 2017
Author
Contributor
To clarify, I think the idiom in that example would make it run correctly whatever the current directory, but I'll check that (and figure out whether popd is needed).
| > .\make html | ||
| in the checkout directory, and again this will write the files to the | ||
| ``_build\html`` directory. |
zware
Sep 28, 2017
Member
The .\ shouldn't even be necessary unless that's a PowerShell-ism; the same command should work on both platforms.
| .. code-block:: ps1con | ||
| PS> Set-ExecutionPolicy -Scope CurrentUser RemoteSigned | ||
zware
Sep 28, 2017
Member
I agree that this probably doesn't need to be here.
By the way, right click the start button for an easy shortcut to Command Prompt (in regular and admin flavors), unless they also removed that in which case I'm never updating my VM again :)
|
@zware So it does. The file explorer version was nice because it opened in the folder you had highlighted, so I'd generally do that 2 or 3 times to get started. Nearly there with a change set satisfying @ezio-melotti . In this section of the devguide you also claim |
|
@zware The |
Whatever the current directory when invoked, the build takes place into _build/... in the directory that contains make.bat, and returns to the invocation directory.
|
Although still showing "changes requested" I believe the review comments are now answered, mostly by change. (Not sure if I've subverted the workflow somehow.) Anyhow, is this good enough to land this now? |
| @@ -39,14 +41,29 @@ if "%1" == "help" ( | |||
| echo. check | |||
| echo. serve to serve devguide on the localhost (8000) | |||
| goto end | |||
| ) | |||
| :help0 | |||
ezio-melotti
Sep 29, 2017
Member
I'm not particularly familiar with bat files, and I'm not sure I understand this change.
ISTM that you just avoid printing the help if the arg is not "help", so the end result should be the same as before.
FWIW all the other commands follow the structure:
if "%1" == "something" (
...
goto end
)
except "check" and "serve".
If possible, to maintain symmetry and improve readability, I would keep/adopt the same structure for all the different targets, and combine with an OR the two targets that result in an help ("help" and ""). This could be done in a separate PR, but unless there's a reason to break this structure, I would either revert or refactor this change.
(I made this comment on the first review, but apparently forgot to save/add it, so I'm adding it back now.)
jeff5
Sep 30, 2017
Author
Contributor
I'm not great with .bat files either, but as briefly stated in the related issue #267, the .\make html currently builds nothing because goto end is executed unconditionally. I attributed this to known issues with parenthetical if bodies and goto, anr the legacy-style work-around made it go away.
On closer inspection, the problem is the un-escaped (8000). I can fix that.
There is no OR operator, but I can make it a bit clearer (closer to what sphinx-quickstart generated).
ezio-melotti
Sep 30, 2017
Member
If there's a valid reason to do it that way, then go ahead, but perhaps add a comment to document it :)
jeff5
Sep 30, 2017
Author
Contributor
Thanks to your challenge I have found the real problem and the right answer.
Previously this disabled subsequent build targets by making "goto end" accidentally unconditional. We also describe the "check" target.
|
Merged, thanks for the PR! |
|
Thanks for the helpful reviews. You may have spotted that my contributions are a side-effect of trying to re-use your work, or follow its model, in a devguide for Jython. I'll offer further things as I spot them. |
Add to the Windows build script a clause to create and use a virtual
environment and install Sphinx there, comparable with the Unix Makefile.
The user may suppress this behaviour by defining a her preferred command
in the environment variable SPHINXBUILD, as before.