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

Re-write userguide to support Python 3 and future proofing Sphinx #2671

Merged
merged 9 commits into from
Apr 15, 2020

Conversation

jreklund
Copy link
Contributor

@jreklund jreklund commented Mar 7, 2020

This are a complete re-write of the Sphinx support for the manual, changes included and not limited too:

  • Changed the requirement to Python 3 and changed conf.py to reflect those changes
  • Contribution documentation and readme
  • Minimal makefile (for building html/epub...)
  • Removed "cilexer" dependency
  • Added requirements.txt for automatic building userguide
  • Removed bundle theme and load it externally with pip
  • Custom Javascript for adding chapter and subject classes to all pages
  • Changed some code styling in the userguide that broke

As discussed in #2644.

Checklist:

  • Securely signed commits
  • User guide updated
  • Conforms to style guide

…ly. Fixed incorrectly rendered userguide pages. [ci skip]
@jreklund jreklund mentioned this pull request Mar 7, 2020
@MGatner
Copy link
Member

MGatner commented Mar 8, 2020

Forgive my ignorance, but let me restate this to see if I have it right.

  1. We use Sphinx to generate HTML content from the user_guide_src code
  2. The styling is handled at least partly by the theme, "sphinx_rtd_theme"
  3. The theme is not compatible with Sphinx version 2 (latest: 2.4.4) so must run on 1.8.5
  4. Sphinx 1.8.5 is not compatible with Python 3

If I got that all right, I'd be in favor of looking into using a more recent branch of the theme, assuming it's compatible with our source code. Looking at their repo there have been hundreds of commits to master since that last release which was more than a year ago. I'd really like to be on Python 3 as I think it would increase our chances or reusable GitHub Action code to assist with automation.

@MGatner
Copy link
Member

MGatner commented Mar 8, 2020

Relevant thread: readthedocs/sphinx_rtd_theme#379

we will not be supporting installation from the repository. We do not want to support users using our master branch, which is not even alpha release quality. We will be releasing to PyPI as development releases more often once some of our structural changes are ironed out. - agjohnson commented on Oct 16, 2019

So that actually suggests sticking with the current release as @jreklund suggested and hoping they have a release soon.

@jreklund
Copy link
Contributor Author

jreklund commented Mar 8, 2020

Just ask away, I figured out this myself yesterday so I'm quite fresh on the subject myself.

  1. That's correct it (with dependencies) turn our rst files into X format. We use it for html and epub.
  2. Correct, that theme provide the base html structure and styling. On top of that we have our own stylesheet.
  3. Correct, the current theme version break the search function. Currently waiting for this to be merged into a release. Remove deprecated use of script_files readthedocs/sphinx_rtd_theme#728
  4. Sphinx 1.8.5 support Python 3, if installed with pip3 instead of pip. But ammaraskar/sphinx-action looks like it runs with pip (Python 2). Sphinx 2 have dropped support for Python 2, don't remember if I got it to run or not, I destroyed like 8 virtual machines yesterday... Will have to check it out again.

Update: PIP (Python 2) will install 1.8.5 as the maximum version. You can however force 1.8.5 on Python 3, but then it depends on ammaraskar/sphinx-action to be compatible as well.

@jreklund
Copy link
Contributor Author

jreklund commented Mar 8, 2020

Okey, made another go at this. We can use Python 3, as long as the virtual machine in actions dosen't restart. We can use the following script to install pip3 and alias it to pip.

pre-build-command: "sudo apt install -y python3-pip && alias pip='pip3'"

And it can install our requirements.

sphinx==1.8.5
sphinxcontrib-phpdomain>=0.7.0
docutils<=0.16

May raise an question about it in the project itself, as their requirements.txt lists 2.2.0 and that not be possible with pip. Only with pip3.

Update
Their travis runs under Python 3 at least, so why aren't my ubuntu getting the correct pip.
https://travis-ci.org/ammaraskar/sphinx-action/jobs/650973162

Update2
Travis does some auto magic with virtual environments, I will switch this PR into Python 3, with pip3. If you don't know how to build a virtual environment we can include in the project?

@MGatner
Copy link
Member

MGatner commented Mar 9, 2020

Actions are not hard to update, might be worth a PR to the owner or a fork to support pip3. I'm not sure about your question in "Update 2" - I wasn't planning on automating anything about the User Guide via Travis, is that still relevant?

Sorry about the merge conflict, I forgot this touched a bunch of pages. I'll look more closely at User Guide updates before this is done.

@jreklund
Copy link
Contributor Author

jreklund commented Mar 9, 2020

I don't know if their actions make some auto-magic like their Travis build. If it does; Python 3 are already being used.

PIP have some functions to create virtual environments, you always get the correct version when building. That's what Travis are using to get a correct Python 3.7 pip build going. So that where my question, if you have build any of those. As you can include it into the project.

At the moment I'm rewriting the manual to support Python 3 instead. Ignoring the virtual environment.

No worries, it happens. Will look at what went wrong.

@jreklund
Copy link
Contributor Author

jreklund commented Mar 9, 2020

I have now updated the manual reflecting Python 3 instead.

@MGatner
Copy link
Member

MGatner commented Mar 12, 2020

I'll make a test Python Action when I have a chance and check out the default versions and what pip is mapped to - then we can decide if using the existing Action makes sense or if we need to fork something.

@@ -53,7 +53,7 @@
# The short X.Y version.
version = '4.0-dev'
# The full version, including alpha/beta/rc tags.
release = '4.0.0-rc.4'
release = '4.0.2'
Copy link
Member

Choose a reason for hiding this comment

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

4.0.3 for next release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I based it upon the admin/version release script. For some reason it didn't run (so I stayed on the current version), but should be run. So 4.0.3. will be added automatically. Or whatever version we bump it up too.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/admin/release#L50

@jreklund jreklund changed the title [ci skip] Removed "cilexer" dependency from sphinx Re-write userguide to support Python 3 and future proofing Sphinx Mar 20, 2020
@jreklund
Copy link
Contributor Author

jreklund commented Mar 20, 2020

Okey, I have now merged in another branch I have been working on. This are no longer a PR for just removing cilexar, but a complete re-write of Sphinx with Python 3 and other goodies.

I have updated the main post and the title to reflect that.

@MGatner
Copy link
Member

MGatner commented Apr 9, 2020

@jreklund This looks amazing, and like a lot of work. It's also totally out of my league. I was trying to hang on this PR but I think I'll need someone else to review. Not sure if @lonnieezell feels comfortable doing that or if we need someone else, but I would like to get this merged in so we can look at automatic UG deployments.

@jreklund
Copy link
Contributor Author

Thanks! It took a fair amount of time to figure it out myself, so I don't expect people to now it themselves. If you want me to clarify some design choices I'm happy to do so. :-)

@lonnieezell
Copy link
Member

I downloaded it and gave it a spin. Had to install pyenv to get away from python 2 (I'm not sure if my company's deploy scripts will work on v3 or not so wanted to make sure to keep both). 3.6 gave me errors, but 3.7 worked great. The UG itself looks great, also. Nice job!

The only thing I can think I'd like to change is to fix the width of the main column to something more readable. The line length is a little outrageous with a big screen.

Merging. Thanks!

@lonnieezell lonnieezell merged commit 7845edc into codeigniter4:develop Apr 15, 2020
@jreklund
Copy link
Contributor Author

Hi, thanks. I can now keep going updating the manual, it where kinda hard too see if everything looked OK, as code didn't receive correct syntax highlight without this (on my machine).

Did you take down notes on what kind of errors you received on Python 3.6? I had no troubles compiling the manual on 3.6.9 on Ubuntu Server, that's the version I got from the OS package manager.

I have had the same problem, and I'm only on a 1080p display. The default value are 800px, but that can be kinda narrow for some of our pages. Personally I think anything above 900px give a bad experience, as you can't see it all at once. But as the theme already had the whole page, I didn't change it.

@lonnieezell
Copy link
Member

Unfortunately I didn't take notes. I think it was a single package that was complaining. I shrugged, realized I wasn't on the latest version of Python and upgraded and it all worked like a charm after that.

Besides readability, I have an ulterior motive - We've been given the chance to get Carbon ads to running, and was hoping to find a nice spot to the side of the text that was out of the way but still above the fold to place a single small ad. But I first need to figure out where the theme actually is lol.

@lonnieezell
Copy link
Member

@jreklund I've been playing a bit and got things working pretty well. I've created a new template that overrides the layout.html file, adding the script tag for the Carbon ads. Have it all styled up and even limited the content width to max of 60em.

Any idea how I could specify that the add only gets inserted when I build? I don't need the ad showing up for everyone as they work on the docs (though I guess that's potentially extra clicks ha!) Trying to find something maybe using environment variables but so far I'm striking out.

@jreklund
Copy link
Contributor Author

I looked up some things today in the docs and it took some time to actually notice that the width of the text had changed. It just seamed so natural.

They only way are to replace / insert that file into the correct folder after the docs have been built, you can't specify different environments and include different files (what I know of). You didn't need to edit conf.py correct? You just added a complete copy of layout.html (with changes) into the correct folder?

Instead of replacing the layout file, what can be done instead are creating a custom JS file, and let it add Carbon <script> instead. So that we don't touch the theme itself, just extending it. You can (if you want) check the url in this JS file and only include it on codeigniter.com. So that users designing and updating the user guide don't get ads.

PS. You have changed the hosting of the userguide correct? From codeigniter/userguide to hosting on codeigniter.com itself?

@lonnieezell
Copy link
Member

Didn't need to edit conf.py. Just added a new file _templates/layout.html with the following:

{% extends "!layout.html" %}

{% block menu %}
    {{ super() }}
    <script async type="text/javascript" src="//cdn.carbonads.com/carbon.js?serve=CE7I62QW&placement=codeignitercom" id="_carbonads_js"></script>
{% endblock %}

And added some styles to citheme.css for the ads.

Don't suppose you can provide an example of inserting that via javascript?

The 4.0 manual is hosted on codeigniter.com now. I cheated a bit last night and used the current version instead of 4.0.2 version, but otherwise, the latest release will be on codeigniter.com and the in development branch should be where it previously was. I would like to look at changing that in the near future, so that all docs are hosted on codeigniter.com with a version switcher, but we're not quite there yet.

@jreklund
Copy link
Contributor Author

jreklund commented Apr 17, 2020

Okey, that looked kinda neat. You didn't need to replace the whole thing. That's good!

If you want to do it JS style. You can modify conf.py:

# A list of JS files.
html_js_files = [
	'js/citheme.js',
	'js/cicarbon.js',
]

source/_static/js/cicarbon.js:

/*
 * Add Carbon Ads underneath navigation to support CodeIgniter Foundation
 */
window.onload = function() {
	// Create a HTML DOM Element Object
	var carbon   = document.createElement('script');
	carbon.async = true;
	carbon.type  = 'text/javascript';
	carbon.src   = 'https://cdn.carbonads.com/carbon.js?serve=CE7I62QW&placement=codeignitercom';
	carbon.id    = '_carbonads_js';

	// Append Carbon Ads to .wy-menu
	document.querySelector('.wy-menu').appendChild(carbon);
}

That would be neat to do it like readthedocs.io and just link to one manual and you can pick any version. They also provide links to download it.

Personally however, I would switch location on where the ad are placed (in my example above I selected the same as you). I would put it in .wy-side-scroll instead, it's more correct regarding the DOM.

@jreklund
Copy link
Contributor Author

jreklund commented Apr 17, 2020

After I have looked thru the userguide with ads on for a while, it's kinda hard to even see it. So we should (if page is wide enough) make it fixed in the right corner.

Here are a mockup.
Application Structure — CodeIgniter4 4 0 2 documentation

@lonnieezell
Copy link
Member

I agree with that. And that was my initial thought. At the time I did that I hadn't yet explored how easy it was to change the theme. But I an put a margin on the main content area for desktops, at least, to keep it workable. I'll have to see how that works on mobile in both of these situations. I didn't actually look into that at all. oops!

If you were willing would you mind taking a look at something else with the docs? There are two things reported on the forums today that I thought were just me, since my connection sucks often. The flicker of unstyled content is the most annoying. I haven't had a chance to dig into what's doing that just yet. But it would be awesome if you were able to!

@jreklund
Copy link
Contributor Author

If you render the ad under the menu, you can use media queries to move it on desktop, if the screen are big enough.

I think the blinking text comes from loading all JS resources before CSS and creating a rendering blockage. There are too many files getting loaded. It's worst in FireFox. Personally I haven't notices anything more than a jumping menu, those + signs in Google Chrome. But now then I start testing other browser, omg! That's horrible.

Will see if I can figure something out.

@jreklund jreklund deleted the sphinx branch April 20, 2020 15:46
@paulbalandan paulbalandan mentioned this pull request Jul 6, 2020
5 tasks
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.

4 participants