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

Fix TMPDIR usage #2204

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Fix TMPDIR usage #2204

merged 2 commits into from
Oct 19, 2023

Conversation

grumpygreenguy
Copy link
Contributor

@grumpygreenguy grumpygreenguy commented Oct 19, 2023

Context

Addressing the temp directory issues reported by @AlseinX.
Our scripts have been overloading the system variable TMPDIR; this had worked until now, but it's clashing with pnpm since the latter relies on os.tmpdir() working properly.

Fix

  • Use a specific name for the variable that designates the installation directory.
    • Drive-by: make sure the directory is actually set (i.e. no defaults)
  • In the few cases where TMPDIR is actually intended to be the catch-all temporary location, set up its creation and cleanup

TODO

  • Make sure CI passes (some steps may fail if they're relying on the default value rather than setting the variable)

@0xverin
Copy link
Contributor

0xverin commented Oct 19, 2023

I tried it locally, and it works:

Setting up worker in tmp/w-0
Copying files from bin
Copying source files to working directory
bin/key.txt does not exist, this is fine, but you can't perform remote attestation with this.
bin/spid.txt does not exist, this is fine, but you can't perform remote attestation with this.
Initialized worker.
Starting worker 0 in background
worker command is: ['./litentry-worker', '--clean-reset', '-P', '2000', '-w', '2001', '-r', '3443', '-h', '4545', '--running-mode', 'mock', '--enable-mock-server', '--parentchain-start-block', '0', '--enable-metrics', 'run', '--skip-ra', '--dev']

Copy link
Contributor

@0xverin 0xverin left a comment

Choose a reason for hiding this comment

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

LGTM.Thanks!

@grumpygreenguy grumpygreenguy merged commit b3a3b63 into dev Oct 19, 2023
27 checks passed
@grumpygreenguy grumpygreenguy deleted the respect-tmpdir branch October 19, 2023 15:15
@Kailai-Wang
Copy link
Collaborator

Kailai-Wang commented Oct 19, 2023

Unfortunately this PR breaks the case where only the parachain is launched without worker, because LITENTRY_PARACHAIN_DIR can only be set from py.

e.g.
make launch-docker-rococo will cause the generated files to be written to /tmp/ directly
make clean-docker-rococo will error out due to unset env - and I don't know how to set this var even if I want to fix it - as the artefacts are located under root /tmp/ :(

make clean-docker-rococo
./scripts/clean-local-docker.sh: line 11: LITENTRY_PARACHAIN_DIR: parameter null or not set
make: *** [Makefile:158: clean-docker-rococo] Error 1

I think we need to get it working.

Other things to check:

  • binary mode should work too
  • multiple copies of worker + parachain should work at the same host

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

Successfully merging this pull request may close these issues.

4 participants