[Docs] Reorganize documentation #1241
Conversation
|
@woju @mkow @aneessahib This is a good starting point, but requires some modifications to adhere to our Gramine code style (and fixing typos, etc.). Could we find a volunteer who could do such amending of this PR to our standards? |
There was a problem hiding this comment.
Reviewed 35 of 35 files at r1, all commit messages.
Reviewable status: all files reviewed, 43 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @Carteepaul)
-- commits line 2 at r1:
This should have a sign-off line with your name and email. And the name and email should correspond to the ones in your GitHub account (you will need to update your GitHub account with this information).
You can browse the commits to see how other people do it: https://github.com/gramineproject/gramine/commits/master
-- commits line 2 at r1:
The title should be something like [Docs] Reorganize documentation
Documentation/attestation.rst line 485 at r1 (raw file):
self-written decoding logic or a standard library function).
This is a spurious change. Please revert it.
Documentation/cloud-deployment.rst line 32 at r1 (raw file):
echo "deb [arch=amd64 signed-by=/usr/share/keyrings/gramine-keyring.gpg] https://packages.gramineproject.io/ $(lsb_release -sc) main" \ | sudo tee /etc/apt/sources.list.d/gramine.list
These are spurious changes, please revert them.
Documentation/concepts-index.rst line 7 at r1 (raw file):
:maxdepth: 1 sgx-intro
Please add a newline here.
Documentation/conf.py line 50 at r1 (raw file):
'sphinx.ext.todo', 'sphinx_rtd_theme', 'sphinx.ext.autosectionlabel',
Hm, what does this extension do? Do we need to install some additional package to have this extension?
Documentation/conf.py line 124 at r1 (raw file):
subprocess.check_call(['doxygen', 'Doxyfile-{}'.format(p)]) html_css_files=['css/gramine.css']
Why did you replace it? What's the difference from app.add_stylesheet?
Documentation/configuration-index.rst line 12 at r1 (raw file):
attestation performance manpages/index.rst
A bit weird. Why do we have manpages under Configuration?
Documentation/contributor-index.rst line 6 at r1 (raw file):
======================= These articles contain helpful material for users who want to contribute to Gramine development.
Please remove all trailing whitespaces from everywhere in your PR.
Documentation/contributor-index.rst line 8 at r1 (raw file):
These articles contain helpful material for users who want to contribute to Gramine development. :doc:`devel/contributing` - Learn about how to report bugs, security vulnerabilities and perform pull requests. This section contains information for working with the Gramine project.
All RST documentation in Gramine should be wrapped at 80 chars per line. Please reformat your documents so.
Documentation/curated-installation.rst line 4 at r1 (raw file):
Ready-made confidential protected images ======================================
You must append two more = chars
Documentation/custom-installation.rst line 3 at r1 (raw file):
.. _custom_installation Install Gramine on your server
I don't like the term server. I would prefer one of these:
systemplatform
Documentation/custom-installation.rst line 4 at r1 (raw file):
Install Gramine on your server =====================================
In RST, the correct formatting is to have as many =, -, etc. characters under the title as there are characters in the title. In this particular case, it should be like this:
Install Gramine on your server
==============================
Ditto for all other places in your PR.
Documentation/developer-index.rst line 14 at r1 (raw file):
devel/new-syscall pal/host-abi python/api
I would rearrange in the order of usefulness:
python/api
devel/debugging
devel/new-syscall
pal/host-abi
Documentation/environment-setup.rst line 4 at r1 (raw file):
Set up the Gramine environment -------------------------------
Please add a new empty line after this one.
Documentation/environment-setup.rst line 32 at r1 (raw file):
gramine-sgx-gen-private-key
No reason to have two empty lines
Documentation/environment-setup.rst line 33 at r1 (raw file):
This command generates an RSA 3072 key suitable for signing SGX enclaves and stores it in HOME/.config/gramine/enclave-key.pem. Protect this key and do not disclose it to anyone.
Directories should be put in backticks. In this particular case:
...stores it in :file:`{HOME}/.config/gramine/enclave-key.pem`.
Documentation/gramine-users.rst line 4 at r1 (raw file):
================ We are excited to share that several companies are experimenting with Gramine for their confidential computing solutions. Please reach out to us at maintainers@gramineproject.io if you are using Gramine and would like to be highlighted on our page. We are looking forward to collaborating with you and continue to enhance Gramine to meet the needs of your confidential computing use cases. We will be updating this list regularly (the list is sorted alphabetically).
Please revert the changes in this file. Seems like the changes were done accidentally.
Documentation/Installation-index.rst line 8 at r1 (raw file):
Choose one of the deployment options based on your business need or preference.
Please remove the two empty lines.
Documentation/management-team.rst line 16 at r1 (raw file):
* Rafał Wojdyła (Invisible Things Lab/Golem) * Mona Vij (Intel) * Isaku Yamahata (Intel)
Hm, now we'll need to modify two files (this and CONTRIBUTING.rst)?
Or do you suggest to remove the maintainers part from CONTRIBUTING.rst?
Documentation/manifest-syntax.rst line 97 at r1 (raw file):
default, or ``fs.start_dir`` if specified). The recommended usage is to provide an absolute path and mount the executable
Please revert all changes in this file. They look accidental.
Documentation/manifest-syntax.rst line 537 at r1 (raw file):
:: sgx.insecure__rpc_thread_num = [NUM]
This is wrong.
Documentation/performance.rst line 184 at r1 (raw file):
You must decide how many untrusted helper RPC threads your application needs. A rule of thumb: specify ``sgx.insecure__rpc_thread_num == sgx.thread_num``, i.e.,
Please revert all changes in this file.
Documentation/prepare-a-signing-key.rst line 8 at r1 (raw file):
These instructions are only required for systems using SGX and have not already created a signing key. - If your system is not using SGX, skip to Run the sample application.
You have Run the sample application in this file, which makes little sense. Maybe merge the contents of this file back into the main file?
Documentation/prepare-a-signing-key.rst line 21 at r1 (raw file):
glibc vs musl
This section doesn't belong in this file.
Documentation/prerequisites.rst line 18 at r1 (raw file):
--------------- To check your hardware and system for SGX compatibility, use the supplied tool, :doc:`manpages/is-sgx-available`. It's installed together with the respective gramine
gramine -> Gramine
Documentation/quickstart.rst line 14 at r1 (raw file):
:doc:`devel/building` - This option is mainly used for assisting in helping the development of Gramine. This option is much more involved. The instructions for this option are listed on another page. ..role:: h1Install Gramine
What's this thing? I mean, what does role do, and why is this necessary?
Documentation/quickstart.rst line 43 at r1 (raw file):
| sudo tee /etc/apt/sources.list.d/intel-sgx.list sudo apt-get update
Why is this line removed?
Documentation/quickstart.rst line 59 at r1 (raw file):
| sudo tee /etc/apt/sources.list.d/intel-sgx.list sudo apt-get update
Why is this line removed?
Documentation/quickstart.rst line 68 at r1 (raw file):
======================== This Gramine image is a minimal distribution of Gramine. It contains only Gramine binaries and tools, as well as the pre-requisite packages to run applications under Gramine. The only currently available Gramine image is based on Ubuntu 20.04. The only requirement on the host system is a Linux kernel with in-kernel SGX driver (available from version 5.11 onward). This Gramine image can be used as a disposable playground environment, to quickly test Gramine with your applications and workloads. This image can also be used as a base for your workflows to produce production-ready Docker images for your SGX applications.
Why do we have the same text in two file?
Documentation/requirements.txt line 2 at r1 (raw file):
sphinx==4.2.0 breathe==4.29.2
Why did you change all these requirements?
Documentation/run-sample-application.rst line 55 at r1 (raw file):
only on Ubuntu). glibc vs musl
Why do we have this in two files?
Documentation/sgx-intro.rst line 363 at r1 (raw file):
:term:`DCAP` Key Separation and Sharing
Please keep the indentation
Documentation/sgx-intro.rst line 393 at r1 (raw file):
This feature was not part of original SGX and therefore not supported by all SGX-enabled hardware.
Please remove this empty line
Documentation/_static/css/gramine.css line 10 at r1 (raw file):
} h1
Why do you need this h1? Why not just use what RestructeredText provides?
Documentation/devel/building.rst line 271 at r1 (raw file):
``libgomp`` is as part of a complete GCC build. .. _FSGSBASE:
What's this? Why is it here?
Documentation/devel/.vscode/settings.json line 2 at r1 (raw file):
{ "esbonio.sphinx.confDir": ""
What's that? Looks useless.
Documentation/img/indx-background.png line 0 at r1 (raw file):
What's that?
Documentation/manpages/gramine-sgx-get-token.rst line 26 at r1 (raw file):
the future, it will be removed altogether.
Please revert changes to this file
Documentation/manpages/index.rst line 9 at r1 (raw file):
:maxdepth: 1 gramine
What's that?
Documentation/manpages/index.rst line 16 at r1 (raw file):
gramine-sgx-ias-request gramine-sgx-ias-verify-report gramine-sgx-quote-dump
We added a new tool in the meantime, and we modified the name of this tool. Please reflect this in the documentation.
See:
- https://gramine.readthedocs.io/en/latest/manpages/gramine-sgx-quote-view.html
- https://gramine.readthedocs.io/en/latest/manpages/gramine-sgx-sigstruct-view.html
Documentation/pal/host-abi.rst line 117 at r1 (raw file):
:project: pal .. doxygenstruct:: pal_initial_mem_range
Why additional indentation?
Documentation/manpages/gramine-sgx-sigstruct.rst line 37 at r1 (raw file):
======= .. code-block:: sh
Please revert changes to this file
There was a problem hiding this comment.
Reviewable status: all files reviewed, 45 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @Carteepaul and @dimakuv)
-- commits line 2 at r1:
Please also include Deb Taylor in the commit message. She played a vital role in the process and did many of the changes. Potentially, her sign-off would also be needed/wanted.
Documentation/management-team.rst line 16 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, now we'll need to modify two files (this and
CONTRIBUTING.rst)?Or do you suggest to remove the maintainers part from
CONTRIBUTING.rst?
This change was originally done by Deb (deb-intel/GramineTest@b09d8d9). Her original commit moved this text completely from CONTRIBUTING.rst to management-team.rst.
Documentation/prepare-a-signing-key.rst line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This section doesn't belong in this file.
I even think that this entire file can be deleted. The upper half is part of environment-setup.rst and the lower half is part of run-sample-application.rst. I also do not see a link to a page with the content of prepare-a-signing-key.rst anywhere.
Documentation/quickstart.rst line 68 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do we have the same text in two file?
Which two files do you mean? At least in this commit, I only can see this text once.
Documentation/requirements.txt line 2 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why did you change all these requirements?
Done by Deb (deb-intel/GramineTest@7d14a6d). We would have to ask her.
Documentation/run-sample-application.rst line 39 at r1 (raw file):
At least in this commit, I only can see this text once.
Documentation/run-sample-application.rst line 55 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do we have this in two files?
Which two files do you mean? At least in this commit, I only can see this text once.
Carteepaul
force-pushed
the
master
branch
2 times, most recently
from
d8cd89f
to
a50d9b2
Compare
There was a problem hiding this comment.
Reviewable status: 16 of 36 files reviewed, 45 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner (waiting on @Carteepaul)
Documentation/requirements.txt line 2 at r1 (raw file):
Previously, BFuhry wrote…
Done by Deb (deb-intel/GramineTest@7d14a6d). We would have to ask her.
How could we ask Deb then?
There was a problem hiding this comment.
Reviewable status: 12 of 36 files reviewed, 49 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @deb-intel and @dimakuv)
Documentation/cloud-deployment.rst line 32 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
These are spurious changes, please revert them.
Done.
Documentation/concepts-index.rst line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a newline here.
Done.
Documentation/conf.py line 48 at r1 (raw file):
'sphinx.ext.intersphinx', 'sphinx.ext.napoleon', 'sphinx.ext.todo',
This extension enables the writer to refer to section headings throughout the document. No additional packages are needed.
Documentation/conf.py line 50 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, what does this extension do? Do we need to install some additional package to have this extension?
Done.
Documentation/conf.py line 124 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why did you replace it? What's the difference from
app.add_stylesheet?
@deb-intel Is there a particular reason for doing that{
Documentation/configuration-index.rst line 12 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
A bit weird. Why do we have manpages under
Configuration?
Done.
Documentation/configuration-index.rst line 11 at r4 (raw file):
manifest-syntax attestation performance
I moved the manpages to their own section.
Documentation/contributor-index.rst line 6 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove all trailing whitespaces from everywhere in your PR.
Done.
Code quote (from Documentation/cloud-deployment.rst):
···Documentation/contributor-index.rst line 8 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
All RST documentation in Gramine should be wrapped at 80 chars per line. Please reformat your documents so.
Done.
Documentation/curated-installation.rst line 4 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You must append two more
=chars
Done.
Code quote (from Documentation/contributor-index.rst):
his page describes the knowledge Documentation/custom-installation.rst line 3 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't like the term
server. I would prefer one of these:
systemplatform
Done.
Documentation/custom-installation.rst line 4 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In RST, the correct formatting is to have as many
=,-, etc. characters under the title as there are characters in the title. In this particular case, it should be like this:Install Gramine on your server ==============================Ditto for all other places in your PR.
I fixed this for all the instances I came across.
Documentation/developer-index.rst line 14 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would rearrange in the order of usefulness:
python/api devel/debugging devel/new-syscall pal/host-abi
I rearranged this list in the order you suggested.
Documentation/environment-setup.rst line 4 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a new empty line after this one.
I added a the line you asked for.
Documentation/environment-setup.rst line 32 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No reason to have two empty lines
I removed the empty lines.
Documentation/environment-setup.rst line 33 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Directories should be put in backticks. In this particular case:
...stores it in :file:`{HOME}/.config/gramine/enclave-key.pem`.
I put the directories in bracket and surrounded them with backticks.
Documentation/gramine-users.rst line 4 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please revert the changes in this file. Seems like the changes were done accidentally.
Done.
Documentation/Installation-index.rst line 8 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove the two empty lines.
Done.
Documentation/management-team.rst line 16 at r1 (raw file):
Previously, BFuhry wrote…
This change was originally done by Deb (deb-intel/GramineTest@b09d8d9). Her original commit moved this text completely from
CONTRIBUTING.rsttomanagement-team.rst.
Done.
Documentation/manifest-syntax.rst line 537 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is wrong.
Done.
Code quote:
sgx.insecure__rpc_thread_num = [NUM]Documentation/performance.rst line 184 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please revert all changes in this file.
Done.
Documentation/prerequisites.rst line 18 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
gramine->Gramine
Done.
Documentation/quickstart.rst line 14 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's this thing? I mean, what does
roledo, and why is this necessary?
Done. I removed this. It was an attempt to create links to a heading from another page.
Documentation/quickstart.rst line 43 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is this line removed?
Done.
Documentation/quickstart.rst line 59 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is this line removed?
Done. Put the line back in.
Documentation/sgx-intro.rst line 363 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please keep the indentation
Done.
Code quote (from Documentation/prepare-a-signing-key.rst):
pare a signing keyDocumentation/sgx-intro.rst line 393 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove this empty line
Done.
Documentation/_static/css/gramine.css line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need this
h1? Why not just use what RestructeredText provides?
Done. This was an attempt to link to a heading in one from another file.
Documentation/devel/building.rst line 271 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's this? Why is it here?
Done.
Documentation/devel/building.rst line 270 at r4 (raw file):
take a long time: unfortunately, the only supported way of building ``libgomp`` is as part of a complete GCC build.
I removed this (.. _FSGSBASE:) .
Documentation/manpages/index.rst line 9 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's that?
Done.
Documentation/manpages/index.rst line 16 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We added a new tool in the meantime, and we modified the name of this tool. Please reflect this in the documentation.
See:
@dimakuv I need help understanding the exact change that needs to be made.
Documentation/manpages/index.rst line 7 at r4 (raw file):
.. toctree:: :maxdepth: 1
This is the index for the manpages.
Documentation/pal/host-abi.rst line 117 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why additional indentation?
Done.
Documentation/attestation.rst line 485 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is a spurious change. Please revert it.
Done.
Documentation/prepare-a-signing-key.rst line 8 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have
Run the sample applicationin this file, which makes little sense. Maybe merge the contents of this file back into the main file?
Done.
Documentation/prepare-a-signing-key.rst line 21 at r1 (raw file):
Previously, BFuhry wrote…
I even think that this entire file can be deleted. The upper half is part of
environment-setup.rstand the lower half is part ofrun-sample-application.rst. I also do not see a link to a page with the content ofprepare-a-signing-key.rstanywhere.
Done. I deleted this file. The information is contained in another file
Documentation/img/indx-background.png line at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's that?
Done. I delete the image.
Documentation/manpages/gramine-sgx-get-token.rst line 26 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please revert changes to this file
Done.
Documentation/manpages/gramine-sgx-sigstruct.rst line 37 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please revert changes to this file
Done.
There was a problem hiding this comment.
Reviewable status: 12 of 36 files reviewed, 49 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @deb-intel and @dimakuv)
Documentation/manifest-syntax.rst line 97 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please revert all changes in this file. They look accidental.
Done.
Documentation/requirements.txt line 2 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
How could we ask Deb then?
@deb-intel Why did we update the requirements?
There was a problem hiding this comment.
Reviewable status: 12 of 36 files reviewed, 48 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @deb-intel and @dimakuv)
Documentation/manifest-syntax.rst line 537 at r1 (raw file):
insecure__rpc_thread_num
Documentation/devel/.vscode/settings.json line 2 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's that? Looks useless.
Done. I removed this file
There was a problem hiding this comment.
Reviewable status: 12 of 36 files reviewed, 48 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @deb-intel and @dimakuv)
Documentation/configuration-index.rst line 11 at r4 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
I moved the manpages to their own section.
I moved the manpages here because I thought having anything to do with help should be in the same section. In prior versions of the documentation, manpages had their own section. It seemed out of place. I moved the man pages to the bottom of the Table of Contents on the side so it would be similar to how it was before I reorganized the documentation.
There was a problem hiding this comment.
Reviewable status: 12 of 36 files reviewed, 48 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @deb-intel and @dimakuv)
Documentation/requirements.txt line 2 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
@deb-intel Why did we update the requirements?
I followed up with Deb on this last week, 4/12. She will get back to me this week.
Carteepaul
changed the title
There was a problem hiding this comment.
Reviewable status: 8 of 36 files reviewed, 48 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner (waiting on @BFuhry, @deb-intel, and @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The title should be something like
[Docs] Reorganize documentation
Done. I renamed the PR to [Doc] Reorganized Documentation
Previously, BFuhry wrote…
Please also include Deb Taylor in the commit message. She played a vital role in the process and did many of the changes. Potentially, her sign-off would also be needed/wanted.
Done. Deb has been included in this process.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This should have a sign-off line with your name and email. And the name and email should correspond to the ones in your GitHub account (you will need to update your GitHub account with this information).
You can browse the commits to see how other people do it: https://github.com/gramineproject/gramine/commits/master
I reviewed these examples. I renamed the commit and will include a sign-off line in each future commit.
Documentation/cloud-deployment.rst line 32 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I reverted these commits.
Documentation/concepts-index.rst line 7 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I added a new line.
Documentation/conf.py line 50 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
This extension enables you to reference a section using its title. We aren't using this, so I deleted it.
Documentation/conf.py line 124 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
@deb-intel Is there a particular reason for doing that{
I'm reaching out to Deb for an answer.
Documentation/contributor-index.rst line 6 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
Benny helped me configure VSCode to recognize unneeded white spaces. I have gone through all of the files to check for trailing whitespace and to check that each file is limited to 80 characters per line.
Documentation/contributor-index.rst line 8 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I have reviewed all the files to check that each is limited to 80 characters per line.
Documentation/curated-installation.rst line 4 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I have gone through each file to check that the heading markers are equal to the length of the heading.
Documentation/custom-installation.rst line 3 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I replace "server" with "system."
Documentation/gramine-users.rst line 4 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I reverted the changes in this file.
Documentation/Installation-index.rst line 8 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I removed the two empty lines.
Documentation/manifest-syntax.rst line 97 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I reverted all these changes.
Documentation/performance.rst line 184 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I reverted all these changes.
Documentation/prerequisites.rst line 18 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I capitalized Gramine.
Documentation/quickstart.rst line 43 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I put the line back in.
Documentation/requirements.txt line 2 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
I followed up with Deb on this last week, 4/12. She will get back to me this week.
Deb changed the requirements in her repo because she needed those versions so she could build the website in her environment. Those requirements are very old and will create documentation issues at some point. I set the requirements back to the original version numbers.
Documentation/sgx-intro.rst line 363 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
Please excuse the comment under Done. It was a mistake.
I invented the phrase to match the rest of the document.
Documentation/sgx-intro.rst line 393 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I removed the empty line.
Documentation/_static/css/gramine.css line 10 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done. This was an attempt to link to a heading in one from another file.
I removed the h1 and the corresponding update in the conf.py file.
Documentation/manpages/index.rst line 9 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I changed the "maxdepth:" to 1 to make it consistent with the rest of the documentation.
Documentation/manpages/index.rst line 16 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
@dimakuv I need help understanding the exact change that needs to be made.
I added "gramine-sgx-sigstruct" to the list and renamed "gramine-sgx-quote-dump" to "gramine-sgx-quote-view"
Documentation/pal/host-abi.rst line 117 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
The indentation was unintentional. I reversed the change.
Documentation/attestation.rst line 485 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I reverted these changes.
Documentation/manpages/gramine-sgx-get-token.rst line 26 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
The changes were reverted.
Documentation/manpages/gramine-sgx-sigstruct.rst line 37 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
I reverted all of the changes in this file.
Carteepaul
force-pushed
the
master
branch
2 times, most recently
from
06c01e2
to
a50d9b2
Compare
There was a problem hiding this comment.
Reviewable status: 4 of 38 files reviewed, 47 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @deb-intel and @dimakuv)
Previously, Carteepaul (Paul Cartee) wrote…
Done. I renamed the PR to [Doc] Reorganized Documentation
@dimakuv Before or during the merge, please include Deb Tyler (@deb-intel) in the PR message.
Documentation/manpages/index.rst line 9 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
I changed the "maxdepth:" to 1 to make it consistent with the rest of the documentation.
@Carteepaul Please check if this change is still part of the repo as mist of your last changes were lost
There was a problem hiding this comment.
Reviewable status: 4 of 38 files reviewed, 46 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @deb-intel and @dimakuv)
Documentation/manpages/index.rst line 9 at r1 (raw file):
Previously, BFuhry wrote…
@Carteepaul Please check if this change is still part of the repo as mist of your last changes were lost
@deb-intel This change is still in the file.
There was a problem hiding this comment.
Reviewed 3 of 15 files at r2, 1 of 11 files at r3, 2 of 4 files at r4, 1 of 21 files at r8, 1 of 11 files at r10, 2 of 18 files at r11, 22 of 24 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 57 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @BFuhry, @Carteepaul, and @deb-intel)
a discussion (no related file):
Is there any website where we can see the rendering of this new version of documentation?
Is it this one: https://carteepaul.github.io/GramineTest/index.html ?
I didn't find manpages on that page.
Documentation/cloud-deployment.rst line 43 at r13 (raw file):
^^^^^^^^^^^^^^^^^^^^^ Only prepare a signing key if you haven't already done so.::
Need to remove the trailing ::
Also, maybe put this sentence in brackets?
Documentation/concepts-index.rst line 8 at r13 (raw file):
sgx-intro
There are three trailing spaces here now.
Documentation/conf.py line 124 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
I'm reaching out to Deb for an answer.
Still no reply. I would prefer to revert this change.
Documentation/contributor-index.rst line 6 at r13 (raw file):
======================= These articles contain helpful material for users who want to contribute to Gramine development.
This line is not wrapped to 80 chars per line.
Documentation/contributor-index.rst line 28 at r13 (raw file):
:doc:`devel/coding-style` - This document describes coding conventions and formatting styles we use in Gramine. All newly committed code must conform to them to pass a review.
Need to add a newline symbol.
Documentation/custom-installation.rst line 13 at r13 (raw file):
For full build instructions, see :doc:`devel/building`.
We have only one empty line between different sections in our docs. So please remove one line here.
Documentation/custom-installation.rst line 22 at r13 (raw file):
requires you to create your own manifest. Cloud cloud-deployment
Cloud cloud?
Documentation/custom-installation.rst line 27 at r13 (raw file):
**Select** :doc:`docker-image-installation` Gramine can be installed in the cloud on VMs that support Intel SGX.
Shouldn't this be moved to under Gramine Docker image section?
Documentation/custom-installation.rst line 31 at r13 (raw file):
**Select** :doc:`cloud-deployment` Refer to the following as you configure and develop Gramine.
I'm a bit confused here. Does this sentence and the below list correspond to the Cloud deployment section? Looks like they shouldn't be. So maybe you need to create one more section here?
Documentation/developer-index.rst line 8 at r13 (raw file):
Helpful material for users who develop Gramine or who are installing Gramine themselves. .. toctree::
Two trailing spaces
Documentation/developer-index.rst line 14 at r13 (raw file):
devel/debugging devel/new-syscall pal/host-abi
We're missing this new file now: https://gramine.readthedocs.io/en/latest/devel/features.html
Documentation/docker-image-installation.rst line 26 at r13 (raw file):
profile using: ``--security-opt seccomp=<profile_file>``
No need for a leading space
Documentation/docker-image-installation.rst line 32 at r13 (raw file):
https://github.com/gramineproject/gramine/blob/master/scripts/docker_seccomp.json. Alternatively you can disable seccomp completely
Add a :
Documentation/environment-setup.rst line 22 at r13 (raw file):
=========================== We supply a tool, `is-sgx-available <https://deb-intel.github.io/GramineTest/manpages/is-sgx-available.html>`_
This is a wrong link. It should be smth like:
:doc:`manpages/is-sgx-available`
See https://raw.githubusercontent.com/gramineproject/gramine/master/Documentation/quickstart.rst for example
Documentation/environment-setup.rst line 37 at r13 (raw file):
This command generates an RSA 3072 key suitable for signing SGX enclaves and stores it in :file: `{HOME}/.config/gramine/enclave-key.pem`.
I don't think it is allowed to have a space between :file: and the directory name.
Documentation/Installation-index.rst line 1 at r13 (raw file):
.. _index_installation
Is this file not used at all? Please remove it.
Documentation/index.rst line 8 at r13 (raw file):
application with minimal host requirements. Gramine can run applications in an isolated environment with benefits comparable to running a complete OS in a virtual machine, including guest customization, ,
Too many commas at the end (, ,)
Documentation/index.rst line 21 at r13 (raw file):
Gramine deployment options ------------------------------------
This file has wrong number of ---, === underlines.
Documentation/index.rst line 21 at r13 (raw file):
Gramine deployment options ------------------------------------
Please add empty lines after section titles, otherwise hard to read.
Documentation/index.rst line 22 at r13 (raw file):
Gramine deployment options ------------------------------------ There are three deployment options for Gramine—each option is described below.
Gramine-each -> Gramine -- each
Documentation/index.rst line 23 at r13 (raw file):
------------------------------------ There are three deployment options for Gramine—each option is described below. There is also one option to help develop Gramine.
I think this sentence doesn't belong here? It's under the section of Gramine deployment options, but refers to a next section Develop Grmaine
Documentation/index.rst line 42 at r13 (raw file):
============================ Docker images are used to run applications in the cloud. The Gramine Shielded
Why is this sentence split into two lines like this?
Documentation/index.rst line 53 at r13 (raw file):
- `Download the Gramine Shielded Container tool <https://github.com/gramineproject/gsc>`_ - Protect the Docker image containing the application you want to protect.
We don't use two empty lines to separate sections in the rest of the documentation. So please unify in this file -- leave only one empty line between sections.
Documentation/index.rst line 61 at r13 (raw file):
Little to no addition modification of your application is needed. These are the processes to follow protect your application with Gramine:
to follow protect -- broken English
Documentation/index.rst line 63 at r13 (raw file):
These are the processes to follow protect your application with Gramine: - :doc:`Install Gramine<quickstart>` - Install Gramine from binaries on to the
on to -- broken English?
Documentation/index.rst line 81 at r13 (raw file):
- :doc:`Build Gramine from source files<devel/building>` - Build Gramine and ensure all the dependencies installed with proper drivers. This option requires a more work but allows you to choose build options.
requires a more work -- broken English
Documentation/index.rst line 82 at r13 (raw file):
ensure all the dependencies installed with proper drivers. This option requires a more work but allows you to choose build options. - :doc:`Set up Debugging<devel/debugging>` - Configure Gramine with Gnu Debugger
Gnu -> GNU
Documentation/index.rst line 82 at r13 (raw file):
ensure all the dependencies installed with proper drivers. This option requires a more work but allows you to choose build options. - :doc:`Set up Debugging<devel/debugging>` - Configure Gramine with Gnu Debugger
Configure Gramine with Gnu Debugger sounds wrong. Why not simply Run Gramine with a debugger.
Documentation/index.rst line 82 at r13 (raw file):
ensure all the dependencies installed with proper drivers. This option requires a more work but allows you to choose build options. - :doc:`Set up Debugging<devel/debugging>` - Configure Gramine with Gnu Debugger
Why Debugging with capital letter?
Documentation/index.rst line 85 at r13 (raw file):
(GDB) and setup compiling optimizations. - :doc:`Implement a new system call<devel/new-syscall>` - Define the interface of the system call, add, import, and Implement new PAL calls if needed.
Implement -- why capital letter?
Documentation/index.rst line 89 at r13 (raw file):
Contribute to Gramine ------------------------------------
Please add a line after this underscore
Documentation/index.rst line 91 at r13 (raw file):
------------------------------------ We encourage anyone who is interested to contribute to Gramine. We offer procedures and user groups that to help you get started.
that to help -- broken English
Documentation/index.rst line 105 at r13 (raw file):
- :doc:`devel/DCO/index` - Affirm that the source code you will submit was originated by you and/or that you have permission to submit it to the Gramine project.
Not 80 chars per line
Documentation/index.rst line 121 at r13 (raw file):
Resources ------------------------------------
Please add a line after this underscore
Documentation/index.rst line 126 at r13 (raw file):
you with any questions you may have. - :doc:`management-team` - This page list the people managing the maintenance of
lists maintainers of Gramine
Documentation/index.rst line 130 at r13 (raw file):
- :doc:`gramine-users` - See what companies are using Gramine for their confidential computing needs - :doc:`glossary` - Become familiar with the terms used for Gramine
Please add dots at the end of sentences in this list.
Documentation/management-team.rst line 16 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Done.
Ok, now we removed this part from the CONTRIBUTING.rst file. I don't mind it.
Documentation/management-team.rst line 27 at r13 (raw file):
from this document. + Additionally, at least 60% (rounded up) of current members have to agree to make any change to the team membership.
Newline symbol should be added
Documentation/manifest-syntax.rst line 135 at r13 (raw file):
containing output of :ref:`gramine-argv-serializer<gramine-argv-serializer>`. If none of the above arguments-handling manifest options are specified in the manifest, the application will get ``argv = [ <libos.entrypoint value> ]``.
Please rewrap to 80 chars per line
Documentation/manifest-syntax.rst line 912 at r13 (raw file):
* ``ocall_inner``: Records enclave state during OCALL. * ``ocall_outer``: Records the outer OCALL function, i.e., what OCALL handlers
To be honest, I would prefer such cosmetic unrelated changes in a separate PR -- the separate PR would be trivial to review and quick to merge. And this PR won't be "polluted" by these unrelated changes.
But whatever.
Documentation/prerequisites.rst line 18 at r13 (raw file):
Check for Intel® SGX compatibility ---------------
Wrong number of underscores
Documentation/run-sample-application.rst line 5 at r13 (raw file):
Run a sample application =======================
Please one more = symbol
Documentation/run-sample-application.rst line 14 at r13 (raw file):
git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/gramine.git
Please remove one empty line
Documentation/run-sample-application.rst line 58 at r13 (raw file):
glibc vs musl ------------------------------------
- Wrong number of underscores
- Please add an empty line after this section title
Documentation/tutorials-index.rst line 10 at r13 (raw file):
tutorials/pytorch/index.rst tutorials/cczoo/index.rst
Please add a newline symbol
Documentation/devel/building.rst line 24 at r13 (raw file):
<https://github.com/gramineproject/gramine/issues/new>`__. **Install from a Docker container**
Why do we add this text here? If user lands on this page, the user wants to build from source, no? I see no reason to tell the user "are you sure you want to build from source? maybe better use our pre-built Docker image?"
Documentation/devel/building.rst line 286 at r13 (raw file):
skip this step. - If your system is using Intel® SGX and have not created a signing key,
have not -> you have not
Documentation/devel/building.rst line 287 at r13 (raw file):
- If your system is using Intel® SGX and have not created a signing key, follow the instructions below.
There is no need to add the indentation before bullet items.
Documentation/devel/building.rst line 287 at r13 (raw file):
- If your system is using Intel® SGX and have not created a signing key, follow the instructions below.
Is this bullet list really needed? It reads like kindergarden explanations...
Documentation/manpages/index.rst line 9 at r13 (raw file):
:maxdepth: 1 gramine
There is no such tool/program as gramine. Please remove it.
Documentation/manpages/index.rst line 20 at r13 (raw file):
gramine-sgx-sign is-sgx-available
Please remove empty line.
BFuhry
force-pushed
the
master
branch
4 times, most recently
from
f9dff9f
to
25cdbae
Compare
There was a problem hiding this comment.
Reviewable status: 20 of 38 files reviewed, 56 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @Carteepaul, @deb-intel, and @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is there any website where we can see the rendering of this new version of documentation?
Is it this one: https://carteepaul.github.io/GramineTest/index.html ?
I didn't find manpages on that page.
A rendered version can be found under: https://carteepaul.github.io/gramine/
You were right, the manpages were missing in the menu. I added the corresponding index.
Documentation/cloud-deployment.rst line 43 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Need to remove the trailing
::Also, maybe put this sentence in brackets?
@dimakuv Which sentence do you want in brackets and why?
Documentation/concepts-index.rst line 8 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There are three trailing spaces here now.
done
Documentation/configuration-index.rst line 11 at r4 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
I moved the manpages here because I thought having anything to do with help should be in the same section. In prior versions of the documentation, manpages had their own section. It seemed out of place. I moved the man pages to the bottom of the Table of Contents on the side so it would be similar to how it was before I reorganized the documentation.
@dimakuv @Carteepaul This page is not linked from anywhere. Does it make any sense? I assume it should be removed
Documentation/contributor-index.rst line 3 at r13 (raw file):
.. _contributor_index Contribution Guidelines
@dimakuv @Carteepaul This page is not linked from anywhere. Does it make any sense? I assume it should be removed
Documentation/contributor-index.rst line 6 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This line is not wrapped to 80 chars per line.
done
Documentation/contributor-index.rst line 28 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Need to add a newline symbol.
done
Documentation/custom-installation.rst line 13 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We have only one empty line between different sections in our docs. So please remove one line here.
Done
Documentation/custom-installation.rst line 22 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Cloud cloud?
Done
Documentation/custom-installation.rst line 27 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Shouldn't this be moved to under
Gramine Docker imagesection?
@dimakuv @Carteepaul I assume this entire file should be removed. It is not linked from anywhere and the content is present on other pages
Documentation/custom-installation.rst line 31 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'm a bit confused here. Does this sentence and the below list correspond to the
Cloud deploymentsection? Looks like they shouldn't be. So maybe you need to create one more section here?
@dimakuv @Carteepaul I assume this entire file should be removed. It is not linked from anywhere and the content is present on other pages
Documentation/developer-index.rst line 8 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Two trailing spaces
Done
Documentation/developer-index.rst line 14 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We're missing this new file now: https://gramine.readthedocs.io/en/latest/devel/features.html
@dimakuv @Carteepaul I assume this entire file should be removed. It is not linked from anywhere
Documentation/docker-image-installation.rst line 26 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No need for a leading space
Done
Documentation/docker-image-installation.rst line 32 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Add a
:
Done
Documentation/environment-setup.rst line 22 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is a wrong link. It should be smth like:
:doc:`manpages/is-sgx-available`See https://raw.githubusercontent.com/gramineproject/gramine/master/Documentation/quickstart.rst for example
Done
Documentation/environment-setup.rst line 37 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't think it is allowed to have a space between
:file:and the directory name.
Done
Documentation/index.rst line 8 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Too many commas at the end (
, ,)
Done
Documentation/index.rst line 21 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This file has wrong number of
---,===underlines.
Done
Documentation/index.rst line 21 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add empty lines after section titles, otherwise hard to read.
Done
Documentation/index.rst line 22 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Gramine-each->Gramine -- each
Done
Even if the length of the line before was the right length to my knowledge.
Documentation/index.rst line 23 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think this sentence doesn't belong here? It's under the section of
Gramine deployment options, but refers to a next sectionDevelop Grmaine
Done
Documentation/index.rst line 42 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is this sentence split into two lines like this?
Done
Documentation/index.rst line 53 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We don't use two empty lines to separate sections in the rest of the documentation. So please unify in this file -- leave only one empty line between sections.
Done
Documentation/index.rst line 61 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
to follow protect-- broken English
done
Documentation/index.rst line 63 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
on to-- broken English?
Done
Documentation/index.rst line 81 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
requires a more work-- broken English
Done
Documentation/index.rst line 82 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Gnu->GNU
Done
Documentation/index.rst line 82 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Configure Gramine with Gnu Debuggersounds wrong. Why not simplyRun Gramine with a debugger.
Done
Documentation/index.rst line 82 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why
Debuggingwith capital letter?
Done
Documentation/index.rst line 85 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Implement-- why capital letter?
Done
Documentation/index.rst line 89 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a line after this underscore
done
Documentation/index.rst line 91 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
that to help-- broken English
done
Documentation/index.rst line 105 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not 80 chars per line
Done
Documentation/index.rst line 121 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a line after this underscore
Done
Documentation/index.rst line 126 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
lists maintainers of Gramine
Done
Documentation/index.rst line 130 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add dots at the end of sentences in this list.
Done
Documentation/management-team.rst line 27 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Newline symbol should be added
Done
Documentation/manifest-syntax.rst line 135 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please rewrap to 80 chars per line
Done
Documentation/manifest-syntax.rst line 912 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
To be honest, I would prefer such cosmetic unrelated changes in a separate PR -- the separate PR would be trivial to review and quick to merge. And this PR won't be "polluted" by these unrelated changes.
But whatever.
I totally agree. This would have been good. But it is too late now to split things again.
Documentation/prerequisites.rst line 18 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wrong number of underscores
Done
Documentation/run-sample-application.rst line 5 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please one more
=symbol
Done
Documentation/run-sample-application.rst line 14 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove one empty line
Done
Documentation/run-sample-application.rst line 58 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
- Wrong number of underscores
- Please add an empty line after this section title
Done
Documentation/tutorials-index.rst line 10 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a newline symbol
Done
Documentation/devel/building.rst line 270 at r4 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
I removed this (.. _FSGSBASE:) .
Done
Documentation/devel/building.rst line 24 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do we add this text here? If user lands on this page, the user wants to build from source, no? I see no reason to tell the user "are you sure you want to build from source? maybe better use our pre-built Docker image?"
Done
I removed it as this text did not make sense. However, we might want to add instructions of how to build Gramine in a Docker container from source. But that is a discussion for another day.
Documentation/devel/building.rst line 286 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
have not->you have not
Done
Documentation/devel/building.rst line 287 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There is no need to add the indentation before bullet items.
Done
Documentation/devel/building.rst line 287 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this bullet list really needed? It reads like kindergarden explanations...
Done
Removed
Documentation/Installation-index.rst line 1 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this file not used at all? Please remove it.
Done
Documentation/manpages/index.rst line 7 at r4 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
This is the index for the manpages.
Done
Documentation/manpages/index.rst line 9 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There is no such tool/program as
gramine. Please remove it.
But the file describing gramine-direct and gramine-sgx is called gramine. Thus, this is correct
Documentation/manpages/index.rst line 20 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove empty line.
Done
There was a problem hiding this comment.
Reviewable status: 20 of 38 files reviewed, 56 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @Carteepaul and @dimakuv)
Documentation/conf.py line 124 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Still no reply. I would prefer to revert this change.
I'm mentioned to @Carteepaul that app.add_stylesheet() is going away. It will be removed in sphinx 4.0 so, at some point, your build will generate errors. Might not be a bad idea to get one of the guys to doublecheck this. Keeping your current preference is fine for now but just note that it might mess up things in the new future.
Documentation/requirements.txt line 2 at r1 (raw file):
Previously, Carteepaul (Paul Cartee) wrote…
Deb changed the requirements in her repo because she needed those versions so she could build the website in her environment. Those requirements are very old and will create documentation issues at some point. I set the requirements back to the original version numbers.
@dimakuv, as @Carteepaul stated, I had updated the requirements in my repo so I could build the website in my environment. @Carteepaul can change it back to whatever requirements you had before. Be aware the your versions of breathe and (especially) sphinx are very old and will at some point create documentation generation issues.
There was a problem hiding this comment.
Reviewable status: 20 of 38 files reviewed, 55 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @deb-intel and @dimakuv)
Documentation/conf.py line 124 at r1 (raw file):
Previously, deb-intel (Deb Taylor) wrote…
I'm mentioned to @Carteepaul that app.add_stylesheet() is going away. It will be removed in sphinx 4.0 so, at some point, your build will generate errors. Might not be a bad idea to get one of the guys to doublecheck this. Keeping your current preference is fine for now but just note that it might mess up things in the new future.
Done. I reverted the changes to the original source.
Documentation/configuration-index.rst line 11 at r4 (raw file):
Previously, BFuhry wrote…
@dimakuv @Carteepaul This page is not linked from anywhere. Does it make any sense? I assume it should be removed
Done. I removed the page. It was a holdover from earlier discussions about reorganizing the docs.
Documentation/contributor-index.rst line 3 at r13 (raw file):
Previously, BFuhry wrote…
@dimakuv @Carteepaul This page is not linked from anywhere. Does it make any sense? I assume it should be removed
Done. I deleted this page. It was another holdover page from our earlier attempts at reorganizing the documentation.
Documentation/custom-installation.rst line 27 at r13 (raw file):
Previously, BFuhry wrote…
@dimakuv @Carteepaul I assume this entire file should be removed. It is not linked from anywhere and the content is present on other pages
Done. I deleted this file. This was another holdover file from our earlier attempts at reorganizing documentation.
Documentation/custom-installation.rst line 31 at r13 (raw file):
Previously, BFuhry wrote…
@dimakuv @Carteepaul I assume this entire file should be removed. It is not linked from anywhere and the content is present on other pages
Done. This file was removed. It was a holdover from our earlier attempts at reorganzing the docs.
Documentation/developer-index.rst line 14 at r13 (raw file):
Previously, BFuhry wrote…
@dimakuv @Carteepaul I assume this entire file should be removed. It is not linked from anywhere
Done. This file was removed.
Documentation/requirements.txt line 2 at r1 (raw file):
Previously, deb-intel (Deb Taylor) wrote…
@dimakuv, as @Carteepaul stated, I had updated the requirements in my repo so I could build the website in my environment. @Carteepaul can change it back to whatever requirements you had before. Be aware the your versions of breathe and (especially) sphinx are very old and will at some point create documentation generation issues.
The requirements were rolled back.
There was a problem hiding this comment.
Reviewed 9 of 15 files at r14, 1 of 2 files at r15, 1 of 1 files at r16, 1 of 1 files at r17, 2 of 2 files at r18, 1 of 1 files at r19, 4 of 4 files at r20, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), period found at the end of a one-liner (waiting on @BFuhry and @Carteepaul)
a discussion (no related file):
Looks good to me now.
We'll need to very carefully rebase this PR to the latest master and verify again that we didn't lose any updated to the documentation.
Documentation/cloud-deployment.rst line 43 at r13 (raw file):
Previously, BFuhry wrote…
@dimakuv Which sentence do you want in brackets and why?
Ah, ignore me. That comment was not important.
Documentation/manpages/index.rst line 9 at r13 (raw file):
Previously, BFuhry wrote…
But the file describing
gramine-directandgramine-sgxis calledgramine. Thus, this is correct
Ah, ok
|
Jenkins, test this please |
There was a problem hiding this comment.
Reviewed 2 of 35 files at r1, 1 of 15 files at r2, 1 of 11 files at r3, 1 of 4 files at r4, 1 of 21 files at r8, 1 of 11 files at r10, 1 of 18 files at r11, 11 of 24 files at r12, 9 of 15 files at r14, 1 of 2 files at r15, 1 of 1 files at r16, 1 of 1 files at r17, 2 of 2 files at r18, 1 of 1 files at r19, 4 of 4 files at r20, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), period found at the end of a one-liner (waiting on @BFuhry and @Carteepaul)
Documentation/curated-installation.rst line 7 at r20 (raw file):
Confidential Compute images with Gramine are ready-made solutions for popular open-source projects such as PyTorch and Redis. Customize your environment
Consider a comma before "such as"
Documentation/curated-installation.rst line 8 at r20 (raw file):
Confidential Compute images with Gramine are ready-made solutions for popular open-source projects such as PyTorch and Redis. Customize your environment through Interactive scripts. The result is a curated, confidentially protected
Errant capitalization. Unless "Interactive scripts" is a proper name.
"confidentially protected" -> "confidentiality-protected"; with the manifest discussion below, is this modifier even needed (or perhaps contradictory)?
Documentation/curated-installation.rst line 10 at r20 (raw file):
through Interactive scripts. The result is a curated, confidentially protected Gramine image that includes your specific machine-learning application, common dependencies, and a manifest file that specifies security policies to enforce
Consider linking to the documentation for policies here
Documentation/docker-image-installation.rst line 1 at r20 (raw file):
Gramine docker image
Capitalize Docker?
Documentation/environment-setup.rst line 14 at r20 (raw file):
- The Intel SGX driver must be built in the Linux kernel; - Intel SGX SDK/PSW and (optionally) Intel DCAP must be installed.
A little odd mix of optionally and must. Consider splitting the bullet
Documentation/manifest-syntax.rst line 135 at r20 (raw file):
containing output of :ref:`gramine-argv-serializer<gramine-argv-serializer>`. If none of the above arguments-handling manifest options are specified in the
arguments -> argument (more idiomatic, at least in US English)
Documentation/prerequisites.rst line 11 at r20 (raw file):
- Linux kernel version at least 5.11 (with Intel® SGX driver enabled); - Intel SGX PSW and (optionally) Intel® DCAP must be installed and configured.
Odd mix of optionally and must. Is this document redundant with environment-setup.rst?
Documentation/devel/building.rst line 276 at r20 (raw file):
The following command generates an |~| RSA 3072 key suitable for signing SGX enclaves and stores it in :file:`{HOME}/.config/gramine/enclave-key.pem`. Protect this key and do not disclose it to anyone::
I count at least 3 instances of these instructions in the PR. Is there any value in consolidating? In the event we need to change something, fewer opportunites to get incoherent.
Documentation/devel/debugging.rst line 57 at r20 (raw file):
Building Gramine with ``--buildtype=debug`` enables debug symbols and GDB integration but disables optimizations. This is usually the right thing to do:
I think the comma helps, but your call.
Documentation/pal/host-abi.rst line 22 at r20 (raw file):
Regardless of the actual implementation, we require PAL to be able to load ELF-format binaries as executables or dynamic libraries and perform the necessary dynamic relocation. PAL needs
I would keep the comma to avoid ambiguity with two conjunctions in the same sentence. Unlike arithmetic, english doesn't have good operator precedence rules :)
There was a problem hiding this comment.
Reviewable status: 36 of 38 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @Carteepaul and @donporter)
Documentation/curated-installation.rst line 20 at r30 (raw file):
- `Redis <https://github.com/gramineproject/contrib/tree/master/Curated-Apps/workloads/redis>`_ - `PyTorch <https://github.com/gramineproject/contrib/tree/master/Curated-Apps/workloads/pytorch>`_
These two links are wrong now.
Documentation/environment-setup.rst line 34 at r30 (raw file):
.. parsed-literal:: gramine-sgx-gen-private-key
This doesn't render correctly: https://carteepaul.github.io/gramine/environment-setup.html#prepare-a-signing-key
Please fix, I think like this is enough:
Only for SGX, and if you haven’t already, enter the following::
gramine-sgx-gen-private-key
(The important parts are the double-colon :: and the 4-spaces indentation.)
Documentation/index.rst line 31 at r30 (raw file):
Confidential compute images are ready-made solutions for popular open source projects such as `PyTorch <https://github.com/gramineproject/contrib/tree/master/Curated-Apps/workloads/pytorch>`_ and `Redis <https://github.com/gramineproject/contrib/tree/master/Curated-Apps/workloads/redis>`_.
These two links are wrong now.
There was a problem hiding this comment.
Reviewable status: 36 of 38 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @Carteepaul, @dimakuv, and @donporter)
Documentation/index.rst line 30 at r30 (raw file):
Confidential compute images are ready-made solutions for popular open source projects such as `PyTorch <https://github.com/gramineproject/contrib/tree/master/Curated-Apps/workloads/pytorch>`_
Code quote:
https://github.com/gramineproject/contrib/tree/master/Curated-Apps/workloads/pytorchDocumentation/index.rst line 31 at r30 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
These two links are wrong now.
Pls kindly see https://github.com/gramineproject/contrib/tree/master/Intel-Confidential-Compute-for-X/workloads/redis
There was a problem hiding this comment.
Reviewable status: 36 of 38 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @Carteepaul, @dimakuv, and @donporter)
Documentation/docker-image-installation.rst line 30 at r30 (raw file):
You can download the profile file from: https://github.com/gramineproject/gramine/blob/master/scripts/docker_seccomp.json.
This is actually a wrapper of docker_seccomp_mar_2021.json.
Should we point to either docker_seccomp_mar_2021.json or docker_seccomp_aug_2022.json which are the real profile files (that people can download from)?
Code quote:
https://github.com/gramineproject/gramine/blob/master/scripts/docker_seccomp.json… and the updates from the team. Signed-off-by: Benny Fuhry <benny.fuhry@intel.com>
There was a problem hiding this comment.
Reviewable status: 33 of 38 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @donporter, and @kailun-qin)
Documentation/curated-installation.rst line 20 at r30 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
These two links are wrong now.
Done
Documentation/docker-image-installation.rst line 30 at r30 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
This is actually a wrapper of
docker_seccomp_mar_2021.json.Should we point to either
docker_seccomp_mar_2021.jsonordocker_seccomp_aug_2022.jsonwhich are the real profile files (that people can download from)?
Whatever is decided about this, please do such content changes after the PR is merged. This content was just moved, but (should be) unchanged (https://gramine.readthedocs.io/en/stable/container-integration.html).
Documentation/environment-setup.rst line 34 at r30 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This doesn't render correctly: https://carteepaul.github.io/gramine/environment-setup.html#prepare-a-signing-key
Please fix, I think like this is enough:
Only for SGX, and if you haven’t already, enter the following:: gramine-sgx-gen-private-key(The important parts are the double-colon
::and the 4-spaces indentation.)
I added a second :, I added the 4-spaces indentation, and I also removed .. parsed-literal:: as you did not mention it. However, it is still not rendered properly.
Documentation/index.rst line 30 at r30 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
done
Documentation/index.rst line 31 at r30 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Pls kindly see https://github.com/gramineproject/contrib/tree/master/Intel-Confidential-Compute-for-X/workloads/redis
done
There was a problem hiding this comment.
I have done the structure changed discussed in the contributor meeting. See rendering of result using the usual link (https://carteepaul.github.io/gramine/)
Reviewable status: 33 of 38 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner (waiting on @dimakuv, @donporter, and @kailun-qin)
Signed-off-by: Benny Fuhry <benny.fuhry@intel.com>
There was a problem hiding this comment.
Reviewable status: 33 of 38 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner (waiting on @Carteepaul, @dimakuv, and @donporter)
Documentation/curated-installation.rst line 15 at r34 (raw file):
open-source projects, such as PyTorch and Redis. Customize your environment through interactive scripts. The result is a curated, confidentially-protected Gramine image that includes
"confidentially-protected" here sounds a bit wrong to me -- it's actually the Gramine instance which is confidentially-protected, rather than the image itself.
Code quote:
confidentially-protected Gramine imageDocumentation/docker-image-installation.rst line 30 at r30 (raw file):
Previously, BFuhry wrote…
Whatever is decided about this, please do such content changes after the PR is merged. This content was just moved, but (should be) unchanged (https://gramine.readthedocs.io/en/stable/container-integration.html).
Right, let's do it in a separate PR (just noticed by chance). Resolving.
Documentation/devel/building.rst line 271 at r34 (raw file):
--------------------- These instructions are only required for systems using Intel® SGX that have not
We have TM indicator here for "Intel® SGX" (full name) but not anywhere else. If I remember correctly, lntel legal review would always require "Intel® SGX" (which is a registered trademark). But should we at least align here and there?
Code quote:
Intel® SGX
There was a problem hiding this comment.
Reviewable status: 33 of 38 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner (waiting on @dimakuv, @donporter, and @kailun-qin)
Documentation/curated-installation.rst line 15 at r34 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
"confidentially-protected" here sounds a bit wrong to me -- it's actually the Gramine instance which is confidentially-protected, rather than the image itself.
This entire text has to be rewritten anyhow. But again, such content changes are for another PR.
Documentation/devel/building.rst line 271 at r34 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
We have TM indicator here for "Intel® SGX" (full name) but not anywhere else. If I remember correctly, lntel legal review would always require "Intel® SGX" (which is a registered trademark). But should we at least align here and there?
You are right. Legal even requires the following on every page where it is used for the first time: Intel® Software Guard Extensions (Intel® SGX)
But again, let's discuss this after the merge and unify it everywhere.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r30, 1 of 2 files at r31, 1 of 1 files at r33, 1 of 1 files at r34, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: OSCAR Lab), period found at the end of a one-liner (waiting on @dimakuv and @kailun-qin)
Documentation/curated-installation.rst line 10 at r20 (raw file):
Previously, BFuhry wrote…
I did not write the text , but I assume you might be misled (due to imprecise text). Each Confidential Compute image has exactly one Gramine manifest file that is prepared, which provides a "specific security policy". We could link to the Gramine manifest syntax, but I don't think that this is what you want. There is no additional security policy mechanism anywhere.
I could change to "... a Gramine manifest file that specifies security policies to enforce for your workload."
The suggested change is clearer. I like your suggestion.
Documentation/curated-installation.rst line 15 at r34 (raw file):
Previously, BFuhry wrote…
This entire text has to be rewritten anyhow. But again, such content changes are for another PR.
I agree with bounding the work on this PR. But I also agree that it should be "confidentiality protected". Note that -ly words are not hyphenated (for some idiosyncratic reason).
… and the updates from the team. Signed-off-by: Benny Fuhry <benny.fuhry@intel.com>
There was a problem hiding this comment.
Reviewed 1 of 2 files at r30, 1 of 1 files at r33, 1 of 2 files at r35, 1 of 1 files at r36, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @BFuhry, @Carteepaul, and @kailun-qin)
Documentation/environment-setup.rst line 34 at r30 (raw file):
Previously, BFuhry wrote…
I added a second
:, I added the 4-spaces indentation, and I also removed.. parsed-literal::as you did not mention it. However, it is still not rendered properly.
You must add another empty line right after the :: line.
Documentation/index.rst line 62 at r36 (raw file):
.. note:: These confidential compute images only run on machines that support Intel SGX.
Actually, what's the point of this note? First of all, it's kinda obvious, second of all, you have the same note in the corresponding file (this one: https://carteepaul.github.io/gramine/curated-installation.html).
There was a problem hiding this comment.
Reviewed 1 of 2 files at r30.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @BFuhry, @Carteepaul, and @kailun-qin)
Documentation/docker-image-installation.rst line 20 at r36 (raw file):
To run the Gramine image via Docker, the recommended command is:: ``docker run --device /dev/sgx_enclave -it gramineproject/gramine``
This is wrong, check how it incorrectly renders: https://carteepaul.github.io/gramine/docker-image-installation.html
What you need to do is to remove backticks, always have :: at the end of the previous paragraph, indent with 4 spaces these code blocks, and have empty lines around them.
In this case:
To run the Gramine image via Docker, the recommended command is::
docker run --device /dev/sgx_enclave -it gramineproject/gramine
If you want...
Documentation/docker-image-installation.rst line 26 at r36 (raw file):
profile using: ``--security-opt seccomp=<profile_file>``
ditto
Documentation/docker-image-installation.rst line 29 at r36 (raw file):
You can download the profile file from:
Why do you need an empty space here? This makes these two paragraphs, but it's actually a single paragraph. Please remove.
Documentation/docker-image-installation.rst line 34 at r36 (raw file):
Alternatively you can disable seccomp completely: ``--security-optseccomp=unconfined``
ditto
… and the updates from the team. Signed-off-by: Benny Fuhry <benny.fuhry@intel.com>
There was a problem hiding this comment.
Reviewable status: 35 of 38 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @donporter, and @kailun-qin)
Documentation/curated-installation.rst line 10 at r20 (raw file):
Previously, donporter (Don Porter) wrote…
The suggested change is clearer. I like your suggestion.
Done
Documentation/curated-installation.rst line 15 at r34 (raw file):
Previously, donporter (Don Porter) wrote…
I agree with bounding the work on this PR. But I also agree that it should be "confidentiality protected". Note that -ly words are not hyphenated (for some idiosyncratic reason).
Done
Documentation/docker-image-installation.rst line 20 at r36 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is wrong, check how it incorrectly renders: https://carteepaul.github.io/gramine/docker-image-installation.html
What you need to do is to remove backticks, always have
::at the end of the previous paragraph, indent with 4 spaces these code blocks, and have empty lines around them.In this case:
To run the Gramine image via Docker, the recommended command is:: docker run --device /dev/sgx_enclave -it gramineproject/gramine If you want...
done
Documentation/docker-image-installation.rst line 26 at r36 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
done
Documentation/docker-image-installation.rst line 29 at r36 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need an empty space here? This makes these two paragraphs, but it's actually a single paragraph. Please remove.
done
Documentation/docker-image-installation.rst line 34 at r36 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
done
Documentation/environment-setup.rst line 34 at r30 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You must add another empty line right after the
::line.
done
Documentation/index.rst line 62 at r36 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, what's the point of this note? First of all, it's kinda obvious, second of all, you have the same note in the corresponding file (this one: https://carteepaul.github.io/gramine/curated-installation.html).
done
There was a problem hiding this comment.
Reviewed 3 of 3 files at r37, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r35, 3 of 3 files at r37, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)
There was a problem hiding this comment.
Reviewed 2 of 35 files at r1, 1 of 15 files at r2, 1 of 21 files at r8, 1 of 11 files at r10, 1 of 18 files at r11, 2 of 24 files at r12, 1 of 31 files at r21, 1 of 29 files at r24, 16 of 28 files at r25, 4 of 4 files at r26, 3 of 4 files at r27, 1 of 2 files at r30, 1 of 2 files at r35, 3 of 3 files at r37, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, period found at the end of a one-liner, "fixup! " found in commit messages' one-liners
Documentation/devel/building.rst line 271 at r34 (raw file):
Previously, BFuhry wrote…
You are right. Legal even requires the following on every page where it is used for the first time: Intel® Software Guard Extensions (Intel® SGX)
But again, let's discuss this after the merge and unify it everywhere.
Sure.
There was a problem hiding this comment.
Reviewed 1 of 21 files at r8, 1 of 11 files at r10, 2 of 24 files at r12, 8 of 28 files at r25, 4 of 4 files at r26, 1 of 4 files at r27, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @Carteepaul)
a discussion (no related file):
I really don't like the state of this PR, it breaks all the rules we have about commits - it's basically a single commit described as "update documentation" changing 500 lines in 22 files, moving files around and doing a lot of unrelated things at the same time. This obscures the commit history and makes it hard to track changes (e.g. to check if we didn't drop some documentation entry accidentally).
So, @BFuhry: Could you split this PR into smaller ones? There are a lot of trivial changes here which will be easy to review and to quickly merge. It shouldn't take much time, but if you don't have enough time for this then we can do this ourselves (preserving proper authorship of the commits).
This way the more disputed changes (like some rewordings) could get a proper review, and it would take less time because we could focus on a single thing at once.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @BFuhry)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
I really don't like the state of this PR, it breaks all the rules we have about commits - it's basically a single commit described as "update documentation" changing 500 lines in 22 files, moving files around and doing a lot of unrelated things at the same time. This obscures the commit history and makes it hard to track changes (e.g. to check if we didn't drop some documentation entry accidentally).
So, @BFuhry: Could you split this PR into smaller ones? There are a lot of trivial changes here which will be easy to review and to quickly merge. It shouldn't take much time, but if you don't have enough time for this then we can do this ourselves (preserving proper authorship of the commits).
This way the more disputed changes (like some rewordings) could get a proper review, and it would take less time because we could focus on a single thing at once.
If @BFuhry doesn't mind, I can take over. I will create a series of small PRs, instead of this single huge PR.
Here is my proposed set of new PRs:
- Moving
Management Teamto a separate doc. - Adding the sidebar (
gramine.css). - Better wording & Typo fixing (manifest, building, PAL ABI, SGX intro, some other docs). This will be one PR with several commits.
- Renames of files and new text with navigation -- the main purpose of this reorganization.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @BFuhry and @Carteepaul)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
If @BFuhry doesn't mind, I can take over. I will create a series of small PRs, instead of this single huge PR.
Here is my proposed set of new PRs:
- Moving
Management Teamto a separate doc.- Adding the sidebar (
gramine.css).- Better wording & Typo fixing (manifest, building, PAL ABI, SGX intro, some other docs). This will be one PR with several commits.
- Renames of files and new text with navigation -- the main purpose of this reorganization.
Ok, first three bullet points are implemented in the following PRs:
- [Docs] Add scrollbar to the side navigation menu #1346
- [Docs] Move "Management Team" to a separate file #1347
- [Docs] Fix typos and improve wording #1348
Please review them. After these PRs are resolved (merged or deemed to be unnecessary), I'll create the final PR that performs the actual reorganization.
Documentation/manifest-syntax.rst line 167 at r37 (raw file):
Unsupported keywords and malformed lines from ``/etc/resolv.conf`` are ignored.
This was a wrong change. While moving these changes to my new PR, I neglected this change.
…e updates from the team.
The final result is rendered here: https://carteepaul.github.io/gramine/
Description of the changes
How to test this PR?
The documentation will be rearranged and contain all of the recent updates by the team.
This change is