8000 Build Instructions: Fix to work with the latest Makefile by davidfstr · Pull Request #2485 · python/peps · GitHub
[go: up one dir, main page]
Skip to content

Build Instructions: Fix to work with the latest Makefile #2485

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 < 8000 a class="Link--inTextBlock" href="https://docs.github.com/terms" target="_blank">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 4 commits into from
Apr 16, 2022
Merged

Build Instructions: Fix to work with the latest Makefile #2485

merged 4 commits into from
Apr 16, 2022

Conversation

davidfstr
Copy link
Contributor

Please note that I did not test the Windows-specific instructions I added, since I do not have quick access to a Windows box.

@davidfstr davidfstr requested a review from a team as a code owner March 30, 2022 13:32
hugovk
hugovk requested changes Mar 30, 2022
View reviewed changes
Copy link
Member
@hugovk hugovk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Likewise, I only checked the non-Windows bit, and looks good.

One suggestion, as there's no output shown, let's remove the $ and use shell instead of console:

-   .. code-block:: console
+   .. code-block:: shell

-      $ make venv
+      make venv
-   .. code-block:: console
+   .. code-block:: shell

-      $ make render
+      make render

This makes it easier to copy and paste the command into the terminal.

Edit: and let's do the same for the other (venv) $ bits on this page.

@davidfstr
Copy link
Contributor Author
davidfstr commented Mar 30, 2022 •
edited
Loading

let's remove the $ and use shell instead of console:

Done, for commands that can be run outside of a venv safely.

Edit: and let's do the same for the other (venv) $ bits on this page.

The (venv) $ is important context to signal that certain code snippets must be run inside a virtual environment. In particular build.py must be. So I didn't apply this change

@CAM-Gerlach
Copy link
Member

I like the changes (since I also prefer shell to console and avoiding $ unless showing output or otherwise necessary, for the reasons mentioned), but if we're going to update those, can we be consistent and update the others in the document as well, if you could @davidfstr?

@hugovk
Copy link
Member
hugovk commented Mar 31, 2022

The (venv) $ is important context to signal that certain code snippets must be run inside a virtual environment. In particular build.py must be. So I didn't apply this change

build.py is runnable outside a virtual environment.

make check-links and make fail-warning use the venv automatically, like make render, so also not needed there.

@davidfstr
Copy link
Contributor Author

build.py is runnable outside a virtual environment.

Not in practice. Here's what happens:

$ python3 build.py 
Traceback (most recent call last):
  File "/Users/me/Projects/peps/build.py", line 10, in <module>
    from sphinx.application import Sphinx
ModuleNotFoundError: No module named 'sphinx'

This is because build.py expects to be run inside the virtual environment where sphinx (and all the other dependencies) are installed. In particular it cannot activate the venv on its own, the way that the Makefile rules can.

So I see a few paths forward:

(1) Alter build.py to support auto-activating the appropriate virtual environment. (May be tricky, especially on Windows.) Then update all of these doc fragments to use shell rather than console.
(2) Revert the doc fragments to all use console consistently. (Status quo)

Comments?

@CAM-Gerlach
Copy link
Member
CAM-Gerlach commented Mar 31, 2022 •
edited
Loading

This is because build.py expects to be run inside the virtual environment where sphinx (and all the other dependencies) are installed. In particular it cannot activate the venv on its own, the way that the Makefile rules can.

It expects to be run inside some environment that has the dependencies in the requirements.txt (i.e. recent versions of Sphinx and Pygments) installed, which may be a venv, conda env, docker container, system Python, separate Python installation, or some other mechanism. This is no different from any Python script that doesn't rely only on the stdlib.

I'm always been a big fan of always using some form of isolation, and typically include at least an admonition and brief instructions on how to create one in the readmes/contributing guide/etc. of the projects I help maintain.

However, especially on a repo whose audience is generally experienced Python developers who understand the basics of dependency management, where on most platforms with make this now handled automatically, where local builds are less necessary when we have online previews and a full CI suite, and where this documentation is itself more developer-focused, I'm not sure we need to prefix every command with (venv), when we already make it more clear in the text and when it isn't necessarily relevant or for a lot of our already narrow audience anyway.

As such, rather than (1) massively overcomplicating build.py with a multi-phase bootstrap to avoid a few experienced users having to activate their environment once per session (which even for me is about once per week, since I keep my PEP terminal tab open perpectually, and I have it automatically activate along with my others on startup), and furthermore potentially causing cause serious problems for anyone that doesn't use the built-in default venv (including @AA-Turner and myself, who both use conda envs), or (2) reverting the improvements here, I suggest (3) Simply consistently porting the other remaining code blocks from console to shell and eliding the (venv) prefix.

@davidfstr davidfstr force-pushed the f/fix_build_instructions branch from 4627cc0 to 13cedbd Compare April 15, 2022 13:38
@davidfstr
Build Instructions: Prefer shell code-blocks over console ones (Part 2)
5dbea7e 
...for all remaining blocks, including those that use build.py
@davidfstr
Copy link
Contributor Author

DId rebase branch to tip of main.

Did highlight that build.py requires activating a virtual environment, and migrated the remaining console-blocks to shell-blocks. I believe this addresses all of @CAM-Gerlach 's feedback.

@CAM-Gerlach CAM-Gerlach added the meta Related to the repo itself and its processes label Apr 16, 2022
CAM-Gerlach
CAM-Gerlach approved these changes Apr 16, 2022
View reviewed changes
Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Checked it over and LGTM, thanks @davidfstr .

@hugovk ?

@hugovk hugovk merged commit adf9512 into python:main Apr 16, 2022
@erlend-aasland erlend-aasland mentioned this pull request May 13, 2022
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jun 27, 2022
Closed
@hugovk hugovk mentioned this pull request Mar 10, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk approved these changes 5BDB

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

Assignees
No one assigned
Labels
CLA signed meta Related to the repo itself and its processes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidfstr @CAM-Gerlach @hugovk @the-knights-who-say-ni
0