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

PEP 545 #11

Merged
merged 4 commits into from Jun 28, 2017
Merged

PEP 545 #11

merged 4 commits into from Jun 28, 2017

Conversation

JulienPalard
Copy link
Member

This implements https://www.python.org/dev/peps/pep-0545/, building all known translations by default (only french for the moment, japanese may arrive soon.

This implements no crosslink between versions (no language switcher in the graphical interface) allowing to test for a period of time before going really public about it.

I tested it with:

$ mkdir /tmp/build_root  /tmp/www_root /tmp/log_root
$ git clone https://github.com/python/cpython.git --branch 3.6 --depth 1 /tmp/build_root/python36
$ git clone https://github.com/python/cpython.git --branch 2.7 --depth 1 /tmp/build_root/python27
$ git clone https://github.com/python/cpython.git --branch 3.5 --depth 1 /tmp/build_root/python35
$ git clone https://github.com/python/cpython.git --branch master --depth 1 /tmp/build_root/python37
$ python3 -m venv /tmp/build_root/environment/
$ . /tmp/build_root/environment/bin/activate
$ python3 -m pip install -r requirements.txt
$ python3 ./build_docs.py --git --build-root /tmp/build_root/ --www-root /tmp/www_root/ --log-directory /tmp/log_root/ --skip-cache-invalidation --group $(id -gn)

And it went well, properly skipping untranslated versions, building a correct hierarchy in www_root. It'll probably miss some work before implementing cross-linking like setting symlinks like /3/, and /dev/, which are currently done manually if I'm right, that I should implement in salt or in this script in a near future.

@JulienPalard
Copy link
Member Author

highlighting @ned-deily @ewdurbin @benjaminp for review.

"""
logging.info("Updating repository %s in %s", repository, directory)
try:
shell_out("git -C {} checkout {}".format(directory, branch))
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to pass a list of arguments rather than a string to avoid issues with shell?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait, the "{}" or "%s" in shell commands are not new: the current code already use them. You can ignore my comment. It can be enhanced in a different PR ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will gladly fix this in another PR yes.

(sphinxbuild, maketarget, os.path.join(log_directory, logname)))
shell_out("cd Doc; make SPHINXBUILD=%s SPHINXOPTS='%s' %s >> %s 2>&1" %
(sphinxbuild, sphinxopts, maketarget,
os.path.join(log_directory, logname)))
Copy link
Member

Choose a reason for hiding this comment

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

Can't use use cwd parameter of subprocess.Popen to avoid the "cd"?

Note: make also has a -C parameter to make the "cd".

Same question for string vs list.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, it will even allow running make directly (without a shell) once your previous comment fixed, which is nice. I tried to keep my changes minimal so I didn't changed this. Will do in another PR, later.

(As I don't have access to the logs or the server, I prefer keeping my chances atomic, so if something break we all know exactly what caused it).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it perfectly makes sense. But it would help if someone could give you access to the logs since you seem to be one of the people working on this project :-)

@ewdurbin: Can you please give access to Julien to the logs? Or maybe send him logs on demand? Thanks.

FYI I proposed to give the commit bit to @JulienPalard on this project :-)
https://mail.python.org/pipermail/python-committers/2017-June/004583.html

Choose a reason for hiding this comment

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

@Haypo I can help get the logs, can you point me to what logs you'll need?

Copy link
Member Author

Choose a reason for hiding this comment

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

@MarkMangoba @Haypo is speaking about the output of this crontab: https://github.com/python/psf-salt/blob/master/salt/docs/init.sls#L47 and /var/log/docsbuild/.

Having a copy of the logs right now is not needed, I'd only need them if I break something in one of my PRs, to diagnose.

Choose a reason for hiding this comment

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

@JulienPalard understood. @ewdurbin do you have a chance to review?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Ok, LGTM.

@vstinner
Copy link
Member

@MarkMangoba, @ewdurbin & others: how can we unblock this situation please?

@ewdurbin ewdurbin merged commit 2921ab3 into python:master Jun 28, 2017
@ewdurbin
Copy link
Member

Running test builds now! Thanks @JulienPalard

@vstinner
Copy link
Member

Cool, thanks @ewdurbin! How can we check for failure? Are you going to notify us? @JulienPalard: maybe join the #python-infra channel on Freenode? ;-)

@ewdurbin
Copy link
Member

@Haypo indeed, unfortunately it's just logs on the docs server when the build job kicks off via cron.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants