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

cmd/wasirun: add --env-inherit flag from wazero #78

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

ydnar
Copy link
Contributor

@ydnar ydnar commented Jul 5, 2023

This enables GOOS=wasip1 GOARCH=wasm go test -exec 'wasirun --env-inherit --dir /' <package>

The go test command sets the working directory AND $PWD to the package source directory when running. This passes PWD through at the time wasirun is executed, rather than the value of PWD at the time go test is executed (typically the module root).

This enables GOOS=wasip1 GOARCH=wasm go test -exec 'wasirun --env-inherit --dir /' <package>

The go test command sets the working directory AND $PWD to the package source directory when running. This passes the PWD variable through at runtime, and not when go test is run.
Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@ydnar
Copy link
Contributor Author

ydnar commented Jul 5, 2023

It took a while to track down why go test -exec couldn't find source-relative test data. Turns out os.Getwd needs $PWD set to work on wasip1.

@achille-roussel achille-roussel merged commit 45bbb61 into dispatchrun:main Jul 5, 2023
2 checks passed
@achille-roussel
Copy link
Contributor

Ah yes, the dependency on PWD is something we should probably have documented properly somewhere when Go 1.21 comes out, thanks for the feedback!

@ydnar
Copy link
Contributor Author

ydnar commented Jul 5, 2023

Wonder if it makes sense to have PWD automatically set, or have a --pwd flag?

I considered augmenting the --dir option to accept a bare environment variable name, which would capture the value from the current wasirun process.

@ydnar ydnar deleted the ydnar/env-inherit branch July 5, 2023 03:33
@achille-roussel
Copy link
Contributor

That would probably be a useful addition, I would advise doing this only if the PWD environment variable does not already exists, then we set it to the current working directory.

Which environment were you running in that did not set PWD?

@ydnar
Copy link
Contributor Author

ydnar commented Jul 5, 2023

We had a Make target that set PWD, but since exec.Cmd doesn’t interpolate shell variables, the interpolation happened in Make. The old, wrong version:

wasm-test:
	$(WASM_ENV) gotip test -v -exec "wasirun --non-blocking-stdio --dir / --env PWD=$$PWD" $(TEST_PACKAGES)

With --env-inherit:

wasm-test:
	$(WASM_ENV) gotip test -v -exec "wasirun --non-blocking-stdio --dir / --env-inherit" $(TEST_PACKAGES)

I think allowing --env to pass the current value of the named variable could be useful:

wasm-test:
	$(WASM_ENV) gotip test -v -exec "wasirun --non-blocking-stdio --dir / --env PWD" $(TEST_PACKAGES)

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.

None yet

2 participants