Skip to content

simplify docbuild: merge Windows and Linux commands. Other minor fixes.#198

Merged
deb-intel merged 3 commits intothesofproject:masterfrom
marc-hb:docbuild-simplify1
Mar 22, 2020
Merged

simplify docbuild: merge Windows and Linux commands. Other minor fixes.#198
deb-intel merged 3 commits intothesofproject:masterfrom
marc-hb:docbuild-simplify1

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Mar 18, 2020

See commit messages for details.

Biggest change is merge Windows and Linux commands into one.

marc-hb added 3 commits March 17, 2020 21:12
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Don't force users to downgrade their python code.
Successfully tested with:
  breathe  4.11.1
  sphinx   2.12
  docutils 0.15.2

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Forward slashes (like "sof/doc") work everywhere on Windows except in
CMD.EXE. As they're passed to ninja or make, they work here - tested.

"dnf install default-jre" doesn't work any more on Fedora 31, replace
with "dnf install java" that does.

Add double backquotes ``monospace`` font in a number of places (badly
needed for --prefix; the double dash was lost)

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb marked this pull request as ready for review March 18, 2020 04:25
docutils==0.14
breathe>=4.9.1
sphinx>=1.7.5
docutils>=0.14
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just noticed Travis relies on this file too. So this change affects CI too, making it less deterministic? If that's considered an issue then maybe we could have two different requirements.txt files?

Copy link
Member

Choose a reason for hiding this comment

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

@xiulipan any comments for travis here ?

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

LGTM, but needs answer on travis.

docutils==0.14
breathe>=4.9.1
sphinx>=1.7.5
docutils>=0.14
Copy link
Member

Choose a reason for hiding this comment

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

@xiulipan any comments for travis here ?

@xiulipan
Copy link
Contributor

@lgirdwood @marc-hb If this will influence the Travis, it will result Travis failed for this PR. I think the Travis survive this change.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Mar 20, 2020

@xiulipan let me expand what I meant.

Looking at https://travis-ci.org/github/thesofproject/sof-docs/builds/663789659 we see that the Travis system downloaded breathe-4.14.1, Sphinx-2.4.4, docutils-0.16, etc.

Tomorrow it may download breathe 5.1.2.3, Sphinx 3.14, docutils-0.42, etc.

The day after it may download higher versions again. Are we happy with that lack of control?

If we are not happy with that, then I can think of two possible solutions:

  • A new scripts/ci-requirements.txt file with ==, or
  • apt-get install python3-breathe python3-sphinx ... # much more stable. New enough?

@xiulipan
Copy link
Contributor

@marc-hb If your change to the requirements.txt with >= is OK to work with manually build. Then I think Travis can also work with that. And if there is any error happen, we need to check if the new version will work with us, then we may need a boundary for the requirements.txt

@marc-hb
Copy link
Collaborator Author

marc-hb commented Mar 20, 2020

then we may need a boundary for the requirements.txt

So let's say for instance we discover in the future that sphinx 3.0 is not compatible. Then we change the requirements.txt to:

sphinx>=1.7.5,<3.0

Sounds good to me. Is this what you meant?

@xiulipan
Copy link
Contributor

@marc-hb Yes, this will also help to check if requirements.txt will work for new users.

Copy link
Collaborator

@deb-intel deb-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@deb-intel deb-intel merged commit ff619a0 into thesofproject:master Mar 22, 2020
@marc-hb marc-hb deleted the docbuild-simplify1 branch March 27, 2020 05:04
@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 4, 2020

So let's say for instance we discover in the future that sphinx 3.0 is not compatible

Sphinx 2 and above were compatible... until PR #207.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 16, 2020

PR #220 switched scripts/requirements.txt to == hardcoding again :-(

@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 16, 2020

Quoting and answering @intelkevinputnam in PR #225

The tech content team are already supporting multiple projects with different dependencies, and we don't have time to vet them all.

So all supported projects are all hardcoded to the same pip versions? Just trying to understand the version control process.

Setting the dependencies explicitly means anyone who clones the repo can get to work right away.

This assumes users don't have another sphinx project hardcoded to different versions. I think you missed my point about "hidden virtualenv requirement".

The latest version of Sphinx on PyPI is actually 3.0.1, so it makes sense to bound it.

>=2.4.4 would install the latest if missing, however it wouldn't change anything to an already installed 2.4.4 (or 2.4.5,...)

Also, Travis-CI depends on the requirements.txt. If upstream changes to the latest version of Sphinx break the build, ...

Very rare. In my experience these projects are very good with backward compatibility.

... a failed deployment might be the first time anyone realizes it.

I don't get this sorry. It doesn't seem to match the older discussion above.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 16, 2020

OK, let me clarify and wrap up my "two files" proposition above:

  1. scripts/requirements.txt that commit 79acd2a just returned to hardcoding, keeps hardcoding with == the official, validated and supported versions.
  2. I submit a new, optional, "best-effort" scripts/softreqs.txt you-name-it file with >= (and less bleeding edge versions than the ones in commit 79acd2a). The purpose of softreqs.txt is not to force users to create a virtualenv for every sphinx project they have.

I can also easily add a second, "sphinx of the day" Travis job that installs with softreqs.txt and flags backward compatibility issues ASAP (The main Travis job doesn't change either).

Please thumb this comment up or down (and explain why if down)

@intelkevinputnam
Copy link
Collaborator

Yes, projects do hard code dependencies. Each tech writer/project does their own thing to manage this. Some use venv (I'm working with another team that insists on this). I have a GUI solution that uses Tox that manages dependencies per project, which explains the .tox addition to conf.py in #219. Other projects use Docker to view PRs built by Jenkins or build locally.

Best practice for docs is not to update to the latest version unless there are specific features needed. It isn't just a matter of features. Changes are made to look and feel that may not be what project wants and may break existing customizations (see static/sof-custom.css).

@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 16, 2020

Thanks @intelkevinputnam for the additional info, very useful. These requirements and additional tooling make sense to me now but they would be too constraining and too high of a setup cost for "drive-by" documentation contributors. I think it's very important that any random person who spots a documentation mistake can quickly build locally a small fix before submitting it. I think my "two requirements.txt" proposition is a simple and good way to address both situations.

The requirements.txt syntax allows comments BTW.

@intelkevinputnam
Copy link
Collaborator

It's fine with me, though this doc set is a little complex for that. The casual user isn't likely going to notice that you need to build the API doxygen content and be confused by all the resulting warnings. If they go to the trouble of doing that, then the additional step of running pip install -r requirements.txt is trivial.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 16, 2020

It's fine with me,

Thanks!

The casual user isn't likely going to notice that you need to build the API doxygen content and be confused by all the resulting warnings.

Already addressed in a97756a. Try it by cleaning sof/doc/ (which is not easy right now because cmake doesn't build out of source, I digress).

BTW the "missing doxygen" warnings are surprisingly clear and easy to dismiss to focus on the other ones.

then the additional step of running pip install -r requirements.txt (with == hardcoding) is trivial.

Running the command is easy, too easy even. Root-causing much later why other, unrelated projects have changed and how is not.

@intelkevinputnam
Copy link
Collaborator

Agreed that switching isn't optimal, which is why I have started recommending the GUI solution (internal only for now) to the tech writers I work with as it creates a Python project-local build environment that updates when the requirements.txt is changed either locally or upstream. Basically invisible venv for tech writers. Which means Python dependency changes are easier to deal with than others. Ninja is a new dependency for generating docs process, right?

It can be tough juggling the needs of both the technical team and the content team, thanks for being flexible.

Do you guys have access to build infrastructure that can keep the artifacts around? Would be nice to simply have each PR built in the cloud and review output there. No local build required.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 16, 2020

Basically invisible virtualenv for tech writers

Sounds cool!

Ninja is a new dependency for generating docs process, right?

Yes and no.

The doxygen guide used to have something like: "switch case Windows: ninja; case Linux: Make" but in this PR and #215 I simplified this to just ninja. ninja is still not a hard requirement for doxygen but it's the easiest and fastest way and the only documented way now. Keep it simple.

Compared to cmake for instance (which is a hard requirement), ninja is now universally used, rock solid and super stable. In general I would classify ninja as a "negligible requirement".

Do you guys have access to build infrastructure that can keep the artifacts around? Would be nice to simply have each PR built in the cloud and review output there.

I don't know, I'm new to this project. I've used exactly that in Zephyr and it was beyond awesome for reviewing documentation changes.

No local build required.

Well, if you don't mind the 5-10 minutes between each typo fix and spamming Travis to the point they throttle us down :-) For instance to debug and fix #225 and thesofproject/sof#2773 I had to build countless times + disable UML with b1f8353

marc-hb added a commit to marc-hb/sof-docs that referenced this pull request Apr 17, 2020
Avoids hardcoding and virtualenvs.

See problem statement and discussion in PR thesofproject#198 and extensive comments
in the new file itself.

Successfully tested with stock, "apt" versions on Ubuntu 18.04 LTS and
a totally empty 'pip3 list --local'

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof-docs that referenced this pull request Apr 17, 2020
Avoids hardcoding and virtualenvs.

See problem statement and discussion in PR thesofproject#198 and extensive comments
in the new file itself.

Successfully tested with stock, "apt" versions on Ubuntu 18.04 LTS and
a totally empty 'pip3 list --local'

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof-docs that referenced this pull request Apr 17, 2020
Avoids hardcoding and virtualenvs.

See problem statement and discussion in PR thesofproject#198 and extensive comments
in the new file itself.

Successfully tested with stock, "apt" versions on Ubuntu 18.04 LTS and
a totally empty 'pip3 list --local'

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 17, 2020

I submit a new, optional, "best-effort" scripts/softreqs.txt you-name-it file with >= (and less bleeding edge versions than the ones in commit 79acd2a). The purpose of softreqs.txt is not to force users to create a virtualenv for every sphinx project they have.

Done in PR #231 , thanks for reviewing.

lgirdwood pushed a commit that referenced this pull request Apr 19, 2020
Avoids hardcoding and virtualenvs.

See problem statement and discussion in PR #198 and extensive comments
in the new file itself.

Successfully tested with stock, "apt" versions on Ubuntu 18.04 LTS and
a totally empty 'pip3 list --local'

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
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.

5 participants