Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate docs for the oak_abi crate #1042

Merged
merged 2 commits into from
Jun 1, 2020

Conversation

rbehjati
Copy link
Contributor

@rbehjati rbehjati commented May 28, 2020

Updates check_docs and build_gh_docs scripts to also check and generate docs
for oak_abi, now that it is a separate crate.

Creates the following hierarchy under the given TARGET_DIR:

├── oak
│   ├── index.html
│   └── doc
├── oak_abi
│   ├── index.html
│   └── doc
├── oak_utils
│    ├── index.html
│    └── doc
└─── index.html

Ref #1037

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

@rbehjati
Copy link
Contributor Author

@daviddrysdale Could you please have a look? I am not very good at bash scripts.

Updates `check_docs` and `build_gh_pages` scripts to also check and generate docs
for `oak_abi`, now that it is a separate crate.
@@ -12,7 +12,7 @@ readonly SCRIPTS_DIR="$(dirname "$0")"
# shellcheck source=scripts/common
source "$SCRIPTS_DIR/common"

readonly TARGET_DIR="${1:-}"
readonly TARGET_DIR="$(cd "${1:-}" && pwd)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what this is doing? Why does it need to cd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had used that to get the absolute path to TARGET_DIR . Changed it to realpath.


# Remove non-doc artifacts from the target dir.
rm --recursive --force "${TARGET_DIR}/debug"
paths=("." "oak_abi" "oak_utils")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these readonly too? Could you add some comments? I guess they are meant to be kept in sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.
Yes, they should be in sync. I could not find a nice way to do this.


# Remove non-deterministic files.
rm "${TARGET_DIR}/.rustc_info.json"
for i in {0..2}; do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is meant to iterate over the entries above, so perhaps make it clearer, e.g. ${#paths[@]} instead of hardcoding 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

for i in {0..2}; do
cd "${CURRENT_DIR}"
target="${TARGET_DIR}/${names[$i]}"
mkdir -p target
Copy link
Collaborator

Choose a reason for hiding this comment

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

mkdir --parent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

for i in {0..2}; do
cd "${CURRENT_DIR}"
target="${TARGET_DIR}/${names[$i]}"
mkdir -p target
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is target meant to be taken literally here? or did you mean $target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

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

I don't think any of us are very good at bash scripts, hence #396 :-)

scripts/check_docs Outdated Show resolved Hide resolved
scripts/check_docs Outdated Show resolved Hide resolved
scripts/build_gh_pages Outdated Show resolved Hide resolved
scripts/build_gh_pages Outdated Show resolved Hide resolved
scripts/build_gh_pages Outdated Show resolved Hide resolved
scripts/build_gh_pages Outdated Show resolved Hide resolved
scripts/build_gh_pages Outdated Show resolved Hide resolved
scripts/build_gh_pages Outdated Show resolved Hide resolved
scripts/build_gh_pages Outdated Show resolved Hide resolved
</body>
</html>
END
done
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to the top-level /index.html file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we now have separate crates, I thought a top-level /index.html would not be needed (in crate.io these would be independent crates).

Copy link
Contributor

Choose a reason for hiding this comment

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

We need a top-level /index.html so that visiting https://project-oak.github.io/oak/ goes somewhere helpful. At the moment I think this will hit a left-over redirect file that redirects to /doc/oak/index.html which won't exist after this PR.

So we could either pick one of the sub-directories to auto-redirect to, or have a page that gives a choice of manual links.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With a top-level index, perhaps we don't need the other index files. Should I remove them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say keep them, so that accidentally landing at (say) https://project-oak.github.io/oak/oak_abi doesn't 404 (assuming that's what would happen otherwise). But either way is fine by me.

<p><a href="./doc/oak/index.html">Oak SDK main page</a></p>
</body>
</html>
# Absolut path to the target directory, to make sure that docs are generated into the correct directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Absolut/Absolute/. Unless we've driven you to drink :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😀

</body>
</html>
END
done
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a top-level /index.html so that visiting https://project-oak.github.io/oak/ goes somewhere helpful. At the moment I think this will hit a left-over redirect file that redirects to /doc/oak/index.html which won't exist after this PR.

So we could either pick one of the sub-directories to auto-redirect to, or have a page that gives a choice of manual links.

scripts/build_gh_pages Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rbehjati rbehjati left a comment

Choose a reason for hiding this comment

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

This PR does not remove the docs for oak_glue because it is still a member of the workspace. But I have tested, and removing oak_glue from the members list in Cargo.toml does remove the docs (I did not make that change here, since it is done in #1036). I noticed that the tests and the examples work fine without oak_glue. Is the plan to remove it entirely?

This PR does not remove the unnecessary documentation for examples either. That is fixed in #1045.

<p><a href="./doc/oak/index.html">Oak SDK main page</a></p>
</body>
</html>
# Absolut path to the target directory, to make sure that docs are generated into the correct directory.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

😀

</body>
</html>
END
done
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With a top-level index, perhaps we don't need the other index files. Should I remove them?

</body>
</html>
END
done
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say keep them, so that accidentally landing at (say) https://project-oak.github.io/oak/oak_abi doesn't 404 (assuming that's what would happen otherwise). But either way is fine by me.

@rbehjati rbehjati merged commit 698b12e into project-oak:master Jun 1, 2020
@rbehjati rbehjati deleted the oak-1037-docs branch June 1, 2020 09:04
@github-actions
Copy link

github-actions bot commented Jun 1, 2020

Reproducibility Index:

4bfb94cd7dd5b2809f0afdebfb5e77c5fbffbbcab5ea8dd7d257e202e80518b4  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
a76f8259ff7200f4c459dd9e071914b2d874cd42677fa964f47f2f1367b80f00  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
93c63ff47b47ead838ca5144547b29463d01d699759b44cc599a3cf70adb9f53  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
5c468b6295f481af8b4b5758e3c67ec5523ecbac99f72a88cf9d43a4a7747274  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
dda50d2185ae1e9c7e9c5cded711823b6952d277ac8f38290eee4093763a67de  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
cb2de9fc1d3af6d4d80b2441409a8b6888b892654e8769253de535cceea78135  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
ffacb1b30bb721d235df5fb648017c8dfaacbb7e318993d16f14893b07293845  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
7ceb8a30ec9bafb671ba15f886a684ec0776c1e00c3bf3ba269a627e938feba3  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
47b81f9c85ad5dfeacb389a6a4a1beb0baaeac828ac002ccd0b04dba8fa07c45  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
95e1b8ed279d5ffa0383773c2ff240560f391173b570628dd21f6ba628c19ee2  ./target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants