-
Notifications
You must be signed in to change notification settings - Fork 891
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
Move creation of env.yaml outside the current directory #14476
Conversation
Could we port this change to other repos as well? We have a new tool rapids-reviser that can help automate that change, @bdice could probably show you how to set this up (although scripting this change is slightly nontrivial relative to our usual since this isn't just a single sed replacement but also a new line addition). |
@@ -6,12 +6,14 @@ set -euo pipefail | |||
rapids-logger "Create test conda environment" | |||
. /opt/conda/etc/profile.d/conda.sh | |||
|
|||
ENV_YAML_DIR="$(mktemp -d)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly necessary since these scripts almost always run in a container, but cleaning up temp directories isn't a bad idea.
ENV_YAML_DIR="$(mktemp -d)" | |
ENV_YAML_DIR="$(mktemp -d)" | |
trap 'rm -rf -- "$ENV_YAML_DIR"' EXIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't temp directories cleared on boot automatically? I'd vote to omit this extra line to keep things as succinct as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus like you mentioned, 99% of the time, these scripts are run in containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks David
I'm fine with getting this merged ASAP, but could we open an issue to track #14476 (comment)? |
/merge |
Description
Moves the creation of the
env.yaml
file in build scripts to a temporary directory instead of the current directory.During the CI builds, a
env.yaml
file is created for use with a mamba call later in the script. This is a problem when trying to reproduce CI locally when the launched docker container tries to create this file locally as root. If the user does not have root access (or equivalent permissions), the file is not created and the process aborts.The same error occurs when creating the
test_results
directory when theRAPIDS_TEST_DIR
is not set. In this case the script tries to create thetest_results
in the current working directory.Checklist