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

use rapids-upload-docs script #1288

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

AyodeAwe
Copy link
Contributor

@AyodeAwe AyodeAwe commented Jun 5, 2023

This PR updates the build_docs.sh script to use the new consolidatory rapids-upload-script shared script.

The shared script enables docs uploads to applicable S3 buckets for branch. nightly and PR builds.

@AyodeAwe AyodeAwe requested a review from a team as a code owner June 5, 2023 18:58
@github-actions github-actions bot added the ci label Jun 5, 2023
@AyodeAwe AyodeAwe added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jun 5, 2023
ci/build_docs.sh Outdated
@@ -26,22 +26,23 @@ rapids-mamba-retry install \
--channel "${PYTHON_CHANNEL}" \
rmm librmm

# Build CPP docs
rapids-logger "Build Doxygen docs"
export RAPIDS_VERSION_NUMBER=${VERSION_NUMBER}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export RAPIDS_VERSION_NUMBER=${VERSION_NUMBER}
export RAPIDS_VERSION_NUMBER=23.08

the VERSION_NUMBER line can now be removed. it was only ever used in the aws s3... commands which are removed in your PR.

please remember to also update the corresponding entry in update-version.sh.

ci/build_docs.sh Outdated
rapids-logger "Build Python docs"
pushd python/docs
sphinx-build -b dirhtml . _html
sphinx-build -b text . _text
mkdir -p "${RAPIDS_DOCS_DIR}/rmm/{html,txt}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mkdir -p "${RAPIDS_DOCS_DIR}/rmm/{html,txt}"
mkdir -p "${RAPIDS_DOCS_DIR}/rmm/"{html,txt}

We'll need to uncomment the {} for this expansion to work properly.

ci/build_docs.sh Outdated
# Build CPP docs
rapids-logger "Build Doxygen docs"
export RAPIDS_VERSION_NUMBER="23.08"
export RAPIDS_DOCS_DIR=${RAPIDS_DOCS_DIR:-"${PWD}/documentation"}
Copy link
Member

@ajschmidt8 ajschmidt8 Jun 5, 2023

Choose a reason for hiding this comment

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

Suggested change
export RAPIDS_DOCS_DIR=${RAPIDS_DOCS_DIR:-"${PWD}/documentation"}
export RAPIDS_DOCS_DIR="$(mktemp -d)"

In hindsight, we can just make this variable a temporary directory.

I originally thought it would be useful to set this environment variable at the shared-workflow level, but the variable's value is only ever used in this particular workflow step.

Therefore we don't need to set it at the workflow level and can simply set it in this script.

I will also open a PR to revert rapidsai/shared-workflows#93.

Copy link
Member

Choose a reason for hiding this comment

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

@ajschmidt8
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 53fa74a into rapidsai:branch-23.08 Jun 5, 2023
41 checks passed
ajschmidt8 added a commit to rapidsai/gha-tools that referenced this pull request Jun 5, 2023
As noted in rapidsai/rmm#1288, the value of
`RAPIDS_DOCS_DIR` is random and generated by `mktemp -d`.

Therefore, it will be useful to print out this directory for users who
are doing local builds according to the repro instructions here:
https://docs.rapids.ai/resources/reproducing-ci/.

Additionally, a `python -m http.server` command is printed so that users
can view the built docs in their browser.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants