Skip to content

Conversation

VickyMerzOwn
Copy link
Contributor

@VickyMerzOwn VickyMerzOwn commented Nov 8, 2022

Felt that Development Environment Setup could have been arranged a bit different.

  • Docker installation can be a prerequisite.
  • ./tools/install_bazel can only be performed from within the repo after cloning it.

This change is Reviewable

Copy link
Contributor

@matzf matzf left a 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.

Copy link
Contributor Author

@VickyMerzOwn VickyMerzOwn left a comment

Choose a reason for hiding this comment

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

:lgtm:

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.

Copy link
Contributor

@matzf matzf left a 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.

Copy link
Contributor Author

@VickyMerzOwn VickyMerzOwn left a 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 the install_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.

Copy link
Contributor Author

@VickyMerzOwn VickyMerzOwn left a 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)

@VickyMerzOwn
Copy link
Contributor Author

I'm sorry about the mess created for such a small thing.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @VickyMerzOwn)

@matzf
Copy link
Contributor

matzf commented Nov 9, 2022

I'm sorry about the mess created for such a small thing.

No, no problem. On the contrary, thanks for following through despite the nit-picks! :)

@matzf matzf enabled auto-merge (squash) November 9, 2022 14:11
@matzf matzf merged commit 3efcbef into scionproto:master Nov 9, 2022
benthor pushed a commit to benthor/scion that referenced this pull request Nov 24, 2022
* 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]
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.

2 participants