West documentation for v1.14 #14983

mbolivar · 2019-03-28T13:31:43Z

This PR adds missing documentation for west. I've also made some fixes and enhancements to the documentation build system which I can take out if that helps.

Fixes: #14992

dbkinder

Lots of reading... but LGTM, thanks!

dbkinder · 2019-03-28T14:30:04Z

doc/extensions/only/eager_only.py

+    # safe both for read and writes (all other operations are local to
+    # the local nodes variable).
+    return {
+        'parallel_read_safe': True,


Thanks for evaluating these Sphinx extensions. We're not running Sphinx with the -j option (yet) because we noticed some differences in the output and error messages, but marking these extensions as thread-safe will help the cause as we evaluate new versions of Sphinx/Breathe...

We're not running Sphinx with the -j option (yet) because we noticed some differences in the output and error messages,

Scary! Here's how I automated the testing of similar issues, this can hopefully provide some "inspiration" for thoroughly testing -j before enabling it: PR #14593

mbolivar · 2019-03-28T15:15:09Z

Lots of reading... but LGTM, thanks!

Thanks! The force push just contains a couple of trivial typo fixes.

Thanks for evaluating these Sphinx extensions.

No problem -- I parallel build here at home and then just ignore the extra errors, so this is a speedup for me.

mbolivar · 2019-03-28T15:59:21Z

aescolar removed the priority: high label 37 minutes ago

@aescolar can you explain this? Missing documentation for a significant new feature seems high priority to me. I'm putting it back. Please add an explanation if you want to remove again.

aescolar · 2019-03-28T16:01:23Z

@mbolivar : "priority:high" was (to the best of my understanding) meant as a label for bugs, not for PRs.
The target milestone indicates when it should be merged

aescolar · 2019-03-28T16:04:34Z

@mbolivar : If the lack of documentation is a high priority bug, then you can tag this PR as bug, and link it to an issue which you can tag as bug & priority:high

mbolivar · 2019-03-28T16:14:03Z

@mbolivar : If the lack of documentation is a bug, then you can tag this PR as bug, and link it to a issue (also labelled as bug) which you can tag as priority:high

OK, I've done that and will remove the tag from this PR. Thanks for the explanation. I didn't see priority being only for issues mentioned in the contribution guidelines (or anywhere else in the documentation) though; where did you get this understanding?

carlescufi · 2019-03-28T16:29:01Z

Makefile

-$(error The ZEPHYR_BASE environment variable must be set)
+WEST := $(shell sh -c "command -v west")
+ifeq ($(strip $(WEST)),)
+$(error The ZEPHYR_BASE environment variable is not set, and west was not found)


This check duplicates the one in doc/CMakeLists.txt, let's get rid of it.

Done, thanks for review!

carlescufi

This is a staggering amount of new documentation. I admit I have only read it briefly without going into all details due to time constraints, but I have made sure that the sections align with my expectations of the west doc.

carlescufi · 2019-03-28T16:41:36Z

@mbolivar thanks for this, it ticks all the checkboxes on the missing parts of west doc!

marc-hb

The cmake variable ZEPHYR_BASE used to be an exact copy of the environment variable ZEPHYR_BASE. This commit turns the cmake variable into something much smarter so I think it should change the name to avoid missing this new smartness and the resulting confusion. I suggest ZEPHYR_TOP but any other (and better) name different from ZEPHYR_BASE would do.

if(DEFINED ENV{ZEPHYR_BASE})
  set(ZEPHYR_TOP $ENV{ZEPHYR_BASE})
elseif(WEST)
  execute_process(
    COMMAND ${WEST} list -f {abspath} zephyr
    OUTPUT_VARIABLE ZEPHYR_TOP)

marc-hb · 2019-03-28T15:33:53Z

doc/extensions/only/eager_only.py

+    # safe both for read and writes (all other operations are local to
+    # the local nodes variable).
+    return {
+        'parallel_read_safe': True,


We're not running Sphinx with the -j option (yet) because we noticed some differences in the output and error messages,

Scary! Here's how I automated the testing of similar issues, this can hopefully provide some "inspiration" for thoroughly testing -j before enabling it: PR #14593

mbolivar · 2019-03-28T17:14:05Z

Thanks @marc-hb!

The cmake variable ZEPHYR_BASE used to be an exact copy of the environment variable ZEPHYR_BASE. This commit turns the cmake variable into something much smarter

Sorry, I don't follow how this is "smarter": it's still the absolute path to the zephyr repository. Can you clarify?

marc-hb

It's "smarter" because it's defined even when the user hasn't defined ZEPHYR_BASE which can be confusing when trying to troubleshoot some issue. Alternative to ZEPHYR_TOP: ZEPHYR_INSTALL.

doc/guides/west/manifest.rst

doc/guides/west/why.rst

marc-hb · 2019-03-28T17:56:33Z

You should "divide and conquer" reviewers by not bundling independent commits in a single PR but scattering them across separate PRs instead. I use a "rebase rotation" to do this. Let's say you have 4 commits to submit:

git show HEAD~3
git push mygithub HEAD~3:fix-parallel-doc-build
git rebase -i HEAD~3 # rotate commits to submit
git show HEAD~3
git push mygithub HEAD~3:build-docs-without-zephyr-base
git rebase -i HEAD~3 # rotate  commits to submit
etc.

Of course you can still submit more than one commit in one PR this way, just git push HEAD~2 instead after some rotation.

mbolivar · 2019-03-28T19:18:11Z

recheck

mbolivar · 2019-03-28T19:59:45Z

It's "smarter" because it's defined even when the user hasn't defined ZEPHYR_BASE which can be confusing when trying to troubleshoot some issue.

I added this patch because I thought it was a trivial improvement that would make people's lives easier, and it's no different than how west itself sets ZEPHYR_BASE for the duration of the call to make commands like west build work without the zephyr-env scripts.

I'll just drop it from this PR since it's not critical to adding the missing west documentation.

zephyrbot · 2019-03-28T20:02:54Z

Found the following issues, please fix and resubmit:

License issues

In most cases you do not need to do anything here, especially if the files
reported below are going into ext/ and if license was approved for inclusion
into ext/ already. Fix any missing license/copyright issues. The license
exception if a JFYI for the maintainers and can be overriden when merging the
pull request.

doc/guides/west/extensions.rst missing license.
doc/guides/west/extensions.rst missing copyright.

mbolivar · 2019-03-28T20:11:15Z

You should "divide and conquer" reviewers by not bundling independent commits in a single PR but scattering them across separate PRs instead.

But then I wouldn't be able to get some of the smaller patches in for v1.14, riding hot on the tails of the commit which really needs merging ;).

More seriously, this series is a gray area for me on the usual "don't mix unrelated commits" hygiene. The main reviewers for all four (now three) patches are David and Carles.

This fixes some high priority issues, including syntax errors in docstrings which prevent us from including API docs in our Sphinx tree. Signed-off-by: Marti Bolivar <[email protected]>

Several of our extensions don't declare they are parallel read or write safe. Upon inspection, they are. Not declaring parallel read safety defeats a lot of the speed ups that are possible when using SPHINXOPTS="-j=auto", so mark the extensions safe and get the performance back. Signed-off-by: Marti Bolivar <[email protected]>

- add glossary terms for important concepts we have to explain often, like "west installation" - add autodoc directives for pulling in west API docs - add missing documentation for built-in features like west's configuration, extension commands, etc. - add missing documentation for "west sign" extension - describe the manifest in a self-contained way rather than linking to the relevant pykwalify schema, also adding a missing reference to "west manifest" in the miscellaneous multi-repo commands list - move various details regarding history and motivation to why.rst among other changes to repo-tool.rst, leaving it closer to a "tell me what I really need to know" style overview - update planned features Fixes: zephyrproject-rtos#14992 Signed-off-by: Marti Bolivar <[email protected]>

marc-hb

Thanks for the recent updates, appreciated!

mbolivar · 2019-03-28T21:07:33Z

Thanks for the recent updates, appreciated!

Sure thing. I see you have resolved all but one of the line comments (#14983 (comment)) and still have a -1 on the PR. Does that mean there's still more to do there?

codecov-io · 2019-03-28T21:17:32Z

Codecov Report

Merging #14983 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #14983   +/-   ##
=======================================
  Coverage   52.92%   52.92%           
=======================================
  Files         309      309           
  Lines       45266    45266           
  Branches    10451    10451           
=======================================
  Hits        23955    23955           
  Misses      16543    16543           
  Partials     4768     4768

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd24b31...95a12a9. Read the comment docs.

marc-hb · 2019-03-28T21:24:37Z

Does that mean there's still more to do there?

No, I thought the -1 was gone. Sorry for the confusion.

dbkinder · 2019-03-29T04:01:33Z

doc/guides/west/extensions.rst

@@ -0,0 +1,244 @@
+.. _west-extensions:


I've got no idea why this .rst file is flagged as missing a license and copyright, unless the python code block on line 77 is somehow noticed?

It's scancode doing that. We can safely ignore it.

carlescufi · 2019-03-29T10:24:27Z

Merging, ignoring the License issue which is a false positive.

mbolivar requested review from nashif, MaureenHelm, tejlmand, dbkinder, carlescufi and galak March 28, 2019 13:31

mbolivar added priority: high High impact/importance bug area: Documentation labels Mar 28, 2019

mbolivar added this to the v1.14.0 milestone Mar 28, 2019

mbolivar requested a review from marc-hb March 28, 2019 14:05

dbkinder approved these changes Mar 28, 2019

View reviewed changes

mbolivar force-pushed the west-docs-v1.14 branch from 90b1216 to 702689a Compare March 28, 2019 15:14

aescolar removed the priority: high High impact/importance bug label Mar 28, 2019

mbolivar added the priority: high High impact/importance bug label Mar 28, 2019

mbolivar mentioned this pull request Mar 28, 2019

projects root should be configurable zephyrproject-rtos/west#237

Closed

mbolivar force-pushed the west-docs-v1.14 branch from 702689a to dafc996 Compare March 28, 2019 16:12

mbolivar removed the priority: high High impact/importance bug label Mar 28, 2019

carlescufi reviewed Mar 28, 2019

View reviewed changes

carlescufi approved these changes Mar 28, 2019

View reviewed changes

tejlmand mentioned this pull request Mar 28, 2019

cmake: Extracted Zephyr module processing into python script #14667

Merged

mbolivar force-pushed the west-docs-v1.14 branch from dafc996 to 2931067 Compare March 28, 2019 17:00

marc-hb requested changes Mar 28, 2019

View reviewed changes

marc-hb reviewed Mar 28, 2019

View reviewed changes

doc/guides/west/manifest.rst Show resolved Hide resolved

doc/guides/west/manifest.rst Show resolved Hide resolved

doc/guides/west/why.rst Outdated Show resolved Hide resolved

doc/guides/west/why.rst Show resolved Hide resolved

mbolivar force-pushed the west-docs-v1.14 branch from 2931067 to c8ded14 Compare March 28, 2019 20:01

mbolivar force-pushed the west-docs-v1.14 branch 3 times, most recently from eeb04b8 to 8dd2517 Compare March 28, 2019 20:16

Marti Bolivar added 2 commits March 28, 2019 14:16

west: move to version v0.5.7

2227a75

This fixes some high priority issues, including syntax errors in docstrings which prevent us from including API docs in our Sphinx tree. Signed-off-by: Marti Bolivar <[email protected]>

mbolivar requested a review from marc-hb March 28, 2019 20:29

mbolivar force-pushed the west-docs-v1.14 branch from 8dd2517 to 95a12a9 Compare March 28, 2019 20:34

marc-hb reviewed Mar 28, 2019

View reviewed changes

marc-hb self-requested a review March 28, 2019 21:23

dbkinder reviewed Mar 29, 2019

View reviewed changes

carlescufi merged commit d3bb3cf into zephyrproject-rtos:master Mar 29, 2019

mbolivar deleted the west-docs-v1.14 branch June 13, 2019 19:36

mbolivar-nordic mentioned this pull request Mar 29, 2021

docs: some cleanup and improvements #33395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

West documentation for v1.14 #14983

West documentation for v1.14 #14983

mbolivar commented Mar 28, 2019 •

edited

Loading

dbkinder left a comment

dbkinder Mar 28, 2019

marc-hb Mar 28, 2019

mbolivar commented Mar 28, 2019

mbolivar commented Mar 28, 2019

aescolar commented Mar 28, 2019 •

edited

Loading

aescolar commented Mar 28, 2019 •

edited

Loading

mbolivar commented Mar 28, 2019 •

edited

Loading

carlescufi Mar 28, 2019

mbolivar Mar 28, 2019

carlescufi left a comment

carlescufi commented Mar 28, 2019

marc-hb left a comment

marc-hb Mar 28, 2019

mbolivar commented Mar 28, 2019

marc-hb left a comment

marc-hb commented Mar 28, 2019

mbolivar commented Mar 28, 2019

mbolivar commented Mar 28, 2019

zephyrbot commented Mar 28, 2019 •

edited

Loading

mbolivar commented Mar 28, 2019

marc-hb left a comment

mbolivar commented Mar 28, 2019

codecov-io commented Mar 28, 2019

marc-hb commented Mar 28, 2019

dbkinder Mar 29, 2019

carlescufi Mar 29, 2019

carlescufi commented Mar 29, 2019

West documentation for v1.14 #14983

West documentation for v1.14 #14983

Conversation

mbolivar commented Mar 28, 2019 • edited Loading

dbkinder left a comment

Choose a reason for hiding this comment

dbkinder Mar 28, 2019

Choose a reason for hiding this comment

marc-hb Mar 28, 2019

Choose a reason for hiding this comment

mbolivar commented Mar 28, 2019

mbolivar commented Mar 28, 2019

aescolar commented Mar 28, 2019 • edited Loading

aescolar commented Mar 28, 2019 • edited Loading

mbolivar commented Mar 28, 2019 • edited Loading

carlescufi Mar 28, 2019

Choose a reason for hiding this comment

mbolivar Mar 28, 2019

Choose a reason for hiding this comment

carlescufi left a comment

Choose a reason for hiding this comment

carlescufi commented Mar 28, 2019

marc-hb left a comment

Choose a reason for hiding this comment

marc-hb Mar 28, 2019

Choose a reason for hiding this comment

mbolivar commented Mar 28, 2019

marc-hb left a comment

Choose a reason for hiding this comment

marc-hb commented Mar 28, 2019

mbolivar commented Mar 28, 2019

mbolivar commented Mar 28, 2019

zephyrbot commented Mar 28, 2019 • edited Loading

License issues

mbolivar commented Mar 28, 2019

marc-hb left a comment

Choose a reason for hiding this comment

mbolivar commented Mar 28, 2019

codecov-io commented Mar 28, 2019

Codecov Report

marc-hb commented Mar 28, 2019

dbkinder Mar 29, 2019

Choose a reason for hiding this comment

carlescufi Mar 29, 2019

Choose a reason for hiding this comment

carlescufi commented Mar 29, 2019

mbolivar commented Mar 28, 2019 •

edited

Loading

aescolar commented Mar 28, 2019 •

edited

Loading

aescolar commented Mar 28, 2019 •

edited

Loading

mbolivar commented Mar 28, 2019 •

edited

Loading

zephyrbot commented Mar 28, 2019 •

edited

Loading