-
Notifications
You must be signed in to change notification settings - Fork 173
{doc/build/setup.rst}: Reorder dev-env setup #4292
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution, @VickyMerzOwn! I've added a few nit-picky remarks.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @VickyMerzOwn)
doc/build/setup.rst
line 8 at r1 (raw file):
Prerequisite -----------
Title underline should match length of title (the spinx build complains otherwise)
doc/build/setup.rst
line 24 at r1 (raw file):
#. Make sure that you are using a clean and recently updated **Ubuntu 18.04**. This environment assumes you're running as a non-root user with ``sudo`` access.
Let's move this point to the Prerequisite(s!) also then. If you feel like it, you can also add a note saying that other versions (or systems) will usually be fine too, but some of the tooling may not work.
doc/build/setup.rst
line 47 at r1 (raw file):
``bazel`` will resolve to the ``bazelisk`` command. #. For this step, make sure you are in the ``scion`` repository root. To install the required dependencies, run:
Let's remove this sentence. This is already required for case in the preceding step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf)
doc/build/setup.rst
line 8 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Title underline should match length of title (the spinx build complains otherwise)
Done.
doc/build/setup.rst
line 24 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Let's move this point to the Prerequisite(s!) also then. If you feel like it, you can also add a note saying that other versions (or systems) will usually be fine too, but some of the tooling may not work.
Done.
doc/build/setup.rst
line 47 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Let's remove this sentence. This is already required for case in the preceding step.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @VickyMerzOwn)
doc/build/setup.rst
line 47 at r1 (raw file):
Previously, VickyMerzOwn (Satvik Vemuganti) wrote…
Done.
Sorry, that's not what I had meant; don't remove the entire step, only remove the sentence "For this step, make sure you are in the scion
repository root. ". We want to keep the install_deps
step, like this:
#. To install the required dependencies, run:
.. code-block:: bash
./tools/install_deps
doc/build/setup.rst
line 12 at r2 (raw file):
Other Ubuntu versions (or systems) will usually be fine too, but some of the tooling may not work. This environment assumes you're running as a non-root user with ``sudo`` access.
Please remove the trailing whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)
doc/build/setup.rst
line 47 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Sorry, that's not what I had meant; don't remove the entire step, only remove the sentence "For this step, make sure you are in the
scion
repository root. ". We want to keep theinstall_deps
step, like this:#. To install the required dependencies, run: .. code-block:: bash ./tools/install_deps
Done.
doc/build/setup.rst
line 12 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Please remove the trailing whitespace.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)
I'm sorry about the mess created for such a small thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @VickyMerzOwn)
No, no problem. On the contrary, thanks for following through despite the nit-picks! :) |
* doc: fix order of steps in dev env setup Installing bazel with the installation script can (obviously) only happen after the repo has been cloned. Move out prerequisites into separate section. [doc]
Felt that Development Environment Setup could have been arranged a bit different.
./tools/install_bazel
can only be performed from within the repo after cloning it.This change is