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

Fix testing instructions #6395

Closed
wants to merge 2 commits into from
Closed

Conversation

michalpipa
Copy link
Contributor

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets

Installation method for Sphinx Extensions for PHP and Symfony was changed from git submodules to pip (62a99b3), but build documentation was never changed. This small fix updates testing instructions.

@xabbuh
Copy link
Member

xabbuh commented Mar 25, 2016

Thanks for catching this @michalpipa. What do you think about also adding the command that you need to execute to install them?

@pasdeloup
Copy link
Contributor

Hi,

I found this error also and was about to send a fix but I saw that there's already this PR.

I have ideas on how to rewrite it: basically, first install "pip" then run "pip install sphinx~=1.3.0 git+https://github.com/fabpot/sphinx-php.git"

I would also add a warning about "pip install": installed binaries may not be in the path if the command is not run as root or via sudo. In this case "make html" doesn't work (it happened to me and as I'm not a Python expert it took me some time to understand where the files were installed)

@michalpipa , do you want me to finish?
@xabbuh , what do you think?

@michalpipa
Copy link
Contributor Author

What do you think about also adding the command that you need to execute to install them?

I think that we should just link to external documentation. There is installation instruction on pip and sphinx-php documentation and I think we don't want to duplicate it (it depends on operating system).

I would also add a warning about "pip install": installed binaries may not be in the path if the command is not run as root or via sudo. In this case "make html" doesn't work (it happened to me and as I'm not a Python expert it took me some time to understand where the files were installed)

Have you read the sphinx-php doc? It says:

You can install the extension by:

  • running sudo pip install git+https://github.com/fabpot/sphinx-php.git;

@stof
Copy link
Member

stof commented Apr 27, 2016

I think that we should just link to external documentation.

The external doc about sphinx will tell you how to install the latest version, and the build currently fails when using 1.4.x

@michalpipa
Copy link
Contributor Author

The external doc about sphinx will tell you how to install the latest version, and the build currently fails when using 1.4.x

Good point. I didn't notice this (I have installed it just a few weeks ago).

So maybe we should link to pip documentation and give specific command for sphinx and sphinx-php?

@michalpipa
Copy link
Contributor Author

My another suggestion is to move this section from "Documentation Format" chapter to "Contributing to the Documentation" chapter and rename it to "Building from source".

Testing is not the only reason to build the documentation and now it is quite hard to find this section.

@pasdeloup
Copy link
Contributor

My another suggestion is to move this section from "Documentation Format" chapter to "Contributing to the Documentation" chapter and rename it to "Building from source".

I agree, that's where I was thinking I would find it.

So maybe we should link to pip documentation and give specific command for sphinx and sphinx-php?

For me the point is to install pip using external documentation (according to your OS) and then use the command that is already used in .travis.yml
pip install sphinx~=1.3.0 git+https://github.com/fabpot/sphinx-php.git

@pasdeloup
Copy link
Contributor

By the way, here was my version:

Nevertheless, if you prefer to do this check locally on your own machine before
submitting your documentation, follow these steps:

* Install pip_ (Pip Installs Python or Pip Installs Packages); on Linux the recommended way is using `Linux Package Managers`_;
* Install Sphinx and Symfony custom extensions using the following command: ``$ pip install sphinx~=1.3.0 git+https://github.com/fabpot/sphinx-php.git``;
* Run ``make html`` and view the generated HTML in the ``_build/html`` directory.

.. caution::

    ``make html`` expect the ``sphinx-build`` binary installed by pip to be available in the PATH. If it is not the case,
    you will have to either add the local target directory in you PATH or launch pip as root or using sudo to install Sphinx globally.

.. _reStructuredText: http://docutils.sourceforge.net/rst.html
.. _pip: https://pip.pypa.io/en/stable/installing/
.. _`Linux Package Managers`: https://packaging.python.org/en/latest/install_requirements_linux/#installing-pip-setuptools-wheel-with-linux-package-managers

@stof
Copy link
Member

stof commented Apr 27, 2016

IMO, putting (Pip Installs Python or Pip Installs Packages) in the sentence just creates confusion

@michalpipa
Copy link
Contributor Author

.. caution::

make html expect the sphinx-build binary installed by pip to be available in the PATH. If it is not the case,
you will have to either add the local target directory in you PATH or launch pip as root or using sudo to install Sphinx globally.

I think we can skip this notice and just say to use "sudo".

@stof
Copy link
Member

stof commented Apr 27, 2016

I think we can skip this notice and just say to use "sudo".

this is not a cross-platform advice. It depends on your system too

@pasdeloup
Copy link
Contributor

this is not a cross-platform advice. It depends on your system too

I lost time because of it so I think it's useful to say, but yes it should be cross-platform so we should not force people to use sudo.
Didn't you have the problem when installing? One of you is using MacOSX? I'm on a fresh installed Ubuntu 16.04

@pasdeloup
Copy link
Contributor

By the way, this "caution" should not block the PR.
Right now the documentation is wrong, so replacing "git submodule update" by "pip install" (with or without sudo) like that would already be better:

Nevertheless, if you prefer to do this check locally on your own machine before
submitting your documentation, follow these steps:

* Install pip_ ; on Linux the recommended way is using `Linux Package Managers`_;
* Install Sphinx and Symfony custom extensions using the following command: ``$ pip install sphinx~=1.3.0 git+https://github.com/fabpot/sphinx-php.git``;
* Run ``make html`` and view the generated HTML in the ``_build/html`` directory.

.. _reStructuredText: http://docutils.sourceforge.net/rst.html
.. _pip: https://pip.pypa.io/en/stable/installing/
.. _`Linux Package Managers`: https://packaging.python.org/en/latest/install_requirements_linux/#installing-pip-setuptools-wheel-with-linux-package-managers

I can propose my caution comment on a later PR for discussion.

@xabbuh
Copy link
Member

xabbuh commented Apr 30, 2016

@pasdeloup Yeah, can you make that change? We can discuss everything else separately then.

@pasdeloup
Copy link
Contributor

pasdeloup commented Apr 30, 2016

It's not my PR, that's why I didn't do the change.
Should I propose my own instead of this one? Sorry, I'm new in contributing so I don't know the usage.

The caution I want to add is only relevant after this PR is merged.

@michalpipa
Copy link
Contributor Author

@xabbuh @pasdeloup @stof

I've updated my pull request. I've added description how to install pip, Sphinx and Sphinx extension. Also moved build instructions from "Documentation Format" to "Contributing to the Documentation".

@@ -274,6 +274,28 @@ page on GitHub and click on ``Details``.
Only Pull Requests to maintained branches are automatically built by
Platform.sh. Check the `roadmap`_ for maintained branches.

Building from source
Copy link
Member

Choose a reason for hiding this comment

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

Source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@pasdeloup
Copy link
Contributor

Hi, I just reinstalled it on Linux Mint and discovered pip 8.x is needed for the ~=1.3 versionning syntax.

You should add something like
**Step 1.** Install pip_ version >8.0 (follow thepip installation_ chapter).

@michalpipa
Copy link
Contributor Author

I just reinstalled it on Linux Mint and discovered pip 8.x is needed for the ~=1.3 versionning syntax.

@pasdeloup Which version do you have?

It works for me on version 1.5.6. It doesn't work on version 1.0, but this version was released in April 2011. So I'm not sure if we care about such an old release (we are talking only about building documentation). I've tried 'sphinx>=1.3.0,==1.3.*' which is sphinx~=1.3 equivalent, but it also doesn't work on pip 1.0 version (lack of * support?).

Again: do we care about pip versions older than 1.5.6?

@pasdeloup
Copy link
Contributor

pasdeloup commented May 6, 2016

@michalpipa using sudo apt-get install python-pip I had 1.5.4 version and the syntax was not working. I checked in the documentation for this version and it was not in it :
https://github.com/pypa/pip/blob/1.5.4/docs/reference/pip_install.rst#id25
First version documenting ~= notation was:
https://github.com/pypa/pip/blob/8.0.0/docs/reference/pip_install.rst#id34

@stof
Copy link
Member

stof commented May 6, 2016

the important thing is not which version documents it, but which version implements it in the code

@stof
Copy link
Member

stof commented May 6, 2016

@michalpipa saying that it works in 1.5.6 is weird. Reading the pip source code, it looks like this feature was added in pip 6.0 (which is the version following 1.5.6, totally logically).

@javiereguiluz
Copy link
Member

@michalpipa thanks for proposing this improvement and thanks @pasdeloup and @stof for the review and discussion.

Sadly this PR cannot be merged because we recently revamped the Symfony Docs. That's why I've recreated this work (and added some minor changes) in #6835. Thanks!

@xabbuh
Copy link
Member

xabbuh commented Aug 3, 2016

Thank you @michalpipa for starting this.

@xabbuh xabbuh closed this Aug 3, 2016
weaverryan added a commit that referenced this pull request Aug 20, 2016
…iluz)

This PR was merged into the 2.7 branch.

Discussion
----------

Updated the instructions to build docs locally

This finishes #6395.

Commits
-------

a2428a4 Minor improvements
914dd30 Fixed the "make" command
77c88bf Fixed again the tricky list syntax
767b49b Fixed the syntax of the list
c71ad4b Minor fixes
a7b4d32 Reword the third step
cebc382 Added the missing link references
348b11c Updated the instructions to build docs locally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants