-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
reST, Sphinx, Read The Docs framework #684
Conversation
Document how to use sphinx and readthedocs for anyone who wants to contribute to the documentation for the project.
This reverts commit 50caa44. I added this during my demo, removing as its not a true prerequisite.
docs/source/installing-brave.rst
Outdated
|
||
echo "deb [arch=amd64] https://brave-browser-apt-dev.s3.brave.com/ `lsb_release -sc` main" | sudo tee -a /etc/apt/sources.list.d/brave-`lsb_release -sc`.list | ||
|
||
cat /etc/apt/sources.list.d/brave-xenial.list |
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.
Is this step necessary? I assume this is part of your workflow for verifying the results of the previous commands, but without an annotation to that effect it seems superfluous. Remove it?
docs/source/installing-brave.rst
Outdated
|
||
|
||
sha256sum brave-browser-dev_0.50.14_amd64.deb | ||
319b925220d07cc4810fd9848a1d09a53d0d66e31e3ea0a8f892336fa26ac1c0 brave-browser-dev_0.50.14_amd64.deb |
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.
These last two steps are equivalent to sha256sum -c brave-browser-dev_0.50.14_amd64.deb.sha256sum
?
docs/source/installing-brave.rst
Outdated
|
||
sudo apt install brave-browser-dev | ||
|
||
To verify checksum:: |
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.
Why is this section included? It verifies a separately downloaded copy of the .deb, which is not equivalent to verifying the .deb that was installed via apt
in the previous instruction. Also, apt
does this for you.
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.
Prefer rebase to merge to keep your PR branch up to date with master.
docs/source/installing-brave.rst
Outdated
|
||
UBUNTU_CODENAME=$( (grep DISTRIB_CODENAME /etc/upstream-release/lsb-release || grep DISTRIB_CODENAME /etc/lsb-release) 2>/dev/null | cut -d'=' -f2 ) | ||
|
||
echo "deb [arch=amd64] https://brave-browser-apt-beta.s3.brave.com/ $UBUNTU_CODENAME main" | sudo tee -a /etc/apt/sources.list.d/brave-$UBUNTU_CODENAME.list |
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.
brave-$UBUNTU_CODENAME.list -> brave-browser-beta-$UBUNTU_CODENAME.list
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.
and same for dev below
Avoid duplicating content in two locations as requested in the comment: #684 (comment)
@mihaiplesa when you get a moment please review my recent updates, I'd like to complete this PR so that we can have it in our master branch asap. Thanks! |
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.
looks good @mbacchi
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.
Looks good, thanks!
Initial framework PR, will add more documentation shortly.
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist: