[Examples] Clean up all examples #2566
Conversation
There was a problem hiding this comment.
Reviewed 47 of 47 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Do we need the "We tested on Ubuntu 18.04 with version "? Does this refer to manual sanity checks that someone did once?
I think what's important is that (most of) examples run on CI, and that's the configuration we can claim to support: Ubuntu 18.04 and 20.04, with the latest version of software available there (at least for the software installed from the repo). And that's something we could mention in a README for all examples, not in individual files.
a discussion (no related file):
Just checking: do we want the README files to be wrapped at 100 columns? That's consistent with everything else, and with what the Emacs settings say, but not with Documentation/.
Examples/README.md, line 8 at r1 (raw file):
please see the README.md in each subdirectory. We recommend to look at the (extensively commented) Redis example to get an idea
This is good advice for understanding Graphene, but as a result people will copy the Redis example with all its comments etc. This might be not what we want users to do, and definitely not what we ourselves want to do when preparing new "official" examples.
I like having one official "template" project that we recommend to start from, but I'd prefer it to be the same as the rest. Maybe the comments and explanations could live in a documentation page, in a separate manifest file, or even in this README?
Examples/curl/README.md, line 6 at r1 (raw file):
Graphene. We tested it with curl 7.58.0 on Ubuntu 18.04. This example uses curl installed on the system instead of compiling from source as some of the other examples do. The Makefile and the template manifest contain comments to
The manifest does not contain comments anymore, right?
Also, I think we don't put double spaces after period.
Examples/memcached/Makefile, line 61 at r1 (raw file):
# The template file contains almost all necessary information to run Memcached under Graphene / # Graphene-SGX. We create memcached.manifest (to be run under non-SGX Graphene) by simply replacing
Thanks for removing "simply"! I try to avoid these too, after I've read this some time ago.
There was a problem hiding this comment.
Reviewable status: 33 of 68 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Do we need the "We tested on Ubuntu 18.04 with version "? Does this refer to manual sanity checks that someone did once?
I think what's important is that (most of) examples run on CI, and that's the configuration we can claim to support: Ubuntu 18.04 and 20.04, with the latest version of software available there (at least for the software installed from the repo). And that's something we could mention in a README for all examples, not in individual files.
OK, I agree that these sentences do not add any value. I will remove them.
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Just checking: do we want the README files to be wrapped at 100 columns? That's consistent with everything else, and with what the Emacs settings say, but not with
Documentation/.
The README files (and actually all such user-text files) must be wrapped at 80 chars. This is Woju's recommendation. The reasoning is that to read text, it is better to have it "single-column" (the less words in one line the better). But for the code, we have 100 chars because overwise there are too many expression breaks.
We may have some README-like files wrapped to 100 chars, this should ideally be re-wrapped to 80 chars. I tried my best to stick to 80 chars in this PR.
a discussion (no related file):
Blocking myself: during final rebase, add that:
- Re-wrapped all READMEs to 80 chars and removed redundant info
- Removed redundant comments from all Makefiles (other than Redis)
Examples/README.md, line 1 at r1 (raw file):
# Example Integrations
FYI: I noticed that this README had strange wrap limit, so re-wrapped to 80 chars.
Examples/README.md, line 8 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
This is good advice for understanding Graphene, but as a result people will copy the Redis example with all its comments etc. This might be not what we want users to do, and definitely not what we ourselves want to do when preparing new "official" examples.
I like having one official "template" project that we recommend to start from, but I'd prefer it to be the same as the rest. Maybe the comments and explanations could live in a documentation page, in a separate manifest file, or even in this README?
But we already have https://graphene.readthedocs.io/en/latest/manifest-syntax.html for explanations. I also dislike "abstract" examples, so I wouldn't want to add a separate manifest file/section in this README.
I added a few words saying that "don't copy these comments in your manifest files".
Examples/curl/README.md, line 6 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
The manifest does not contain comments anymore, right?
Also, I think we don't put double spaces after period.
Done.
There was a problem hiding this comment.
Reviewed 35 of 35 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The README files (and actually all such user-text files) must be wrapped at 80 chars. This is Woju's recommendation. The reasoning is that to read text, it is better to have it "single-column" (the less words in one line the better). But for the code, we have 100 chars because overwise there are too many expression breaks.
We may have some README-like files wrapped to 100 chars, this should ideally be re-wrapped to 80 chars. I tried my best to stick to 80 chars in this PR.
OK. Adding that to Emacs config: PR #2568
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
OK, I agree that these sentences do not add any value. I will remove them.
That looks better, but can you also remove them from the manifest templates? A lot of them begin with "This manifest was prepared and tested on Ubuntu 18.04".
Examples/README.md, line 8 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But we already have https://graphene.readthedocs.io/en/latest/manifest-syntax.html for explanations. I also dislike "abstract" examples, so I wouldn't want to add a separate manifest file/section in this README.
I added a few words saying that "don't copy these comments in your manifest files".
OK, that helps.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
That looks better, but can you also remove them from the manifest templates? A lot of them begin with "This manifest was prepared and tested on Ubuntu 18.04".
But I did... See my last fixup commit.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But I did... See my last fixup commit.
The last commit I see is bff01a6, and git grep "prepared and tested" still shows the manifest templates for me.
There was a problem hiding this comment.
Reviewable status: 46 of 68 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
The last commit I see is bff01a6, and
git grep "prepared and tested"still shows the manifest templates for me.
Done. Sorry, misunderstood you. We were talking about READMEs, so I only removed in READMEs. Now also removed in manifests.
There was a problem hiding this comment.
Reviewed 22 of 22 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. Sorry, misunderstood you. We were talking about READMEs, so I only removed in READMEs. Now also removed in manifests.
Good to see all this gone!
|
Jenkins, retest Jenkins-Debug-18.04 please (timeout on |
There was a problem hiding this comment.
Reviewed 11 of 47 files at r1, 35 of 35 files at r2, 22 of 22 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Blocking myself: during final rebase, add that:
- Re-wrapped all READMEs to 80 chars and removed redundant info
- Removed redundant comments from all Makefiles (other than Redis)
And that you removed 2 examples
Examples/README.md, line 12 at r3 (raw file):
how to write the README, Makefile and manifest files. If you take this example as a template, we recommend to remove the comments from your copies as they only add noise.
This is completely missing the context IMO :) What you say here is about the case when someone wants to create and commit a new example to the repo, while the examples are mostly for the users which just try to use Graphene for their workloads.
Examples/README.md, line 13 at r3 (raw file):
as a template, we recommend to remove the comments from your copies as they only add noise.
Could you mention that most of the examples use oversimplified configuration which is not secure? (e.g. specifying many security-critical files as allowed_files)
Examples/README.md, line 14 at r3 (raw file):
add noise. ## How to Contribute?
We'll need to update this, but we can do this after repo migration.
Examples/bash/Makefile, line 8 at r3 (raw file):
# - make SGX=1 DEBUG=1 # # Use `make clean` to remove Graphene-generated files.
This should actually be moved to the readme in all examples, I think.
Examples/blender/blender.manifest.template, line 1 at r3 (raw file):
# INSECURE!!!
These should actually stay IMO. They are example-specific and important for the people which will use it.
Examples/memcached/memcached.manifest.template, line 104 at r3 (raw file):
# typical Memcached workloads. # # NOTE: Memcached does not fail explicitly when enclave memory is exhausted.
This may be important and is example-specific.
Examples/ra-tls-secret-prov/README.md, line 11 at r3 (raw file):
`Pal/src/host/Linux-SGX/tools/ra-tls`. Additionally, mbedTLS libraries are required. For ECDSA/DCAP attestation, the DCAP software infrastructure must be installed and working correctly on the host.
it was already there, but I think it should be work instead of working
Examples/memcached/README.md, line 41 at r3 (raw file):
host-OS username
Hmm, this is a very questionable wording :) Graphene emulates Linux, so it shouldn't be this way and may be surprising to the reader. And here it's this way because we have very limited uid/gid emulation in usermode and hack-around this limitation in this example's configuration by mounting host's /etc/passwd.
Examples/redis/README.md, line 10 at r3 (raw file):
and requirements for applications running under Graphene-SGX. If using these files as templates for your own applications, please remove the comments in your copies as they only add noise.
This isn't required and sounds a bit weird to say. If someone is customizing the configuration for their own application then it's obvious that they can drop the comments.
There was a problem hiding this comment.
Reviewable status: 48 of 69 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
a discussion (no related file):
And that you removed 2 examples
This is a separate (first) commit in this PR: 1397269
Examples/README.md, line 12 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This is completely missing the context IMO :) What you say here is about the case when someone wants to create and commit a new example to the repo, while the examples are mostly for the users which just try to use Graphene for their workloads.
Done.
Examples/README.md, line 13 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you mention that most of the examples use oversimplified configuration which is not secure? (e.g. specifying many security-critical files as
allowed_files)
Done.
Examples/bash/Makefile, line 8 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This should actually be moved to the readme in all examples, I think.
Done.
Examples/blender/blender.manifest.template, line 1 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
These should actually stay IMO. They are example-specific and important for the people which will use it.
Done
Examples/memcached/memcached.manifest.template, line 104 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This may be important and is example-specific.
Done.
Examples/ra-tls-secret-prov/README.md, line 11 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
it was already there, but I think it should be
workinstead ofworking
Done.
Examples/memcached/README.md, line 41 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
host-OS usernameHmm, this is a very questionable wording :) Graphene emulates Linux, so it shouldn't be this way and may be surprising to the reader. And here it's this way because we have very limited uid/gid emulation in usermode and hack-around this limitation in this example's configuration by mounting host's
/etc/passwd.
Done. Hopefully this wording is less confusing.
Examples/redis/README.md, line 10 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This isn't required and sounds a bit weird to say. If someone is customizing the configuration for their own application then it's obvious that they can drop the comments.
Done.
There was a problem hiding this comment.
Reviewed 21 of 21 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
And that you removed 2 examples
This is a separate (first) commit in this PR: 1397269
Ah, I missed it.
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
- Redundant comments are removed from all Makefiles and manifest files except Redis. - Mentions of deprecated Ubuntu 16.04 are removed from all examples. - All readme files are now `README.md`. - Re-wrapped all readme files to 80 chars and removed redundant info. - Simplified/removed misc manifest options in some examples. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
dimakuv
force-pushed
the
dimakuv/examples-rm-comments
branch
from
55ac005 to
b2a8eee
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Ah, I missed it.
Done.
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
|
Jenkins, retest Jenkins-Debug-20.04 please (another series of timeouts on random LTP tests, unrelated to this PR but worrying, we need to debug this) |
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
Description of the changes
README.md.How to test this PR?
CI is enough. I tested a couple examples manually. But there should be zero actual changes.
This change is