Skip to content

tinygo: set cmd.Dir even when running emulators #2387

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

Merged
merged 2 commits into from
Dec 20, 2021

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Dec 14, 2021

This allows compress/bzip2 to pass with -target=wasi

Fixes #2367

This allows compress/bzip2 to pass with -target=wasi

Fixes tinygo-org#2367
@niaow niaow self-requested a review December 14, 2021 22:56
@dkegel-fastly
Copy link
Contributor

LGTM. Rescues GOOS=windows tinygo test debug/macho , too!

@niaow
Copy link
Member

niaow commented Dec 15, 2021

Huh. . . I actually seem to still get an access error:

panic: open testdata/e.txt.bz2: errno 2

@niaow
Copy link
Member

niaow commented Dec 15, 2021

This also seems to prevent wasm (js wasm) from even starting at all, since it seems to use a relative path:

Error: Cannot find module '/usr/lib/go/src/compress/bzip2/targets/wasm_exec.js'

@dgryski
Copy link
Member Author

dgryski commented Dec 15, 2021

Hmm.. can we use os.ExpandEnv to handle constructing the path here?

aykevl
aykevl previously requested changes Dec 17, 2021
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

This also seems to prevent wasm (js wasm) from even starting at all, since it seems to use a relative path:

Error: Cannot find module '/usr/lib/go/src/compress/bzip2/targets/wasm_exec.js'

You are correct, this is a regression.
The easiest solution may be to use the same {root} pattern as is used in CFlags. See for example:

"cflags": [
"-DNRF51",
"-I{root}/lib/CMSIS/CMSIS/Include",
"-I{root}/lib/nrfx/mdk"
],

@dgryski
Copy link
Member Author

dgryski commented Dec 17, 2021

Looks like {root} isn't expanded everywhere:

Error: Cannot find module '/Users/dgryski/go/src/go.googlesource.com/go/src/compress/bzip2/{root}/targets/wasm_exec.js'

Where would be the best place to handle that substitution?

@niaow
Copy link
Member

niaow commented Dec 18, 2021

It looks like we handle it for CFLAGS in compileopts/config.go when transforming the config values. It might make sense to put it there as well?

@dgryski
Copy link
Member Author

dgryski commented Dec 18, 2021

I was starting to look at compileopts.LoadTarget where we already do a bit of munging (for the asyncify extra files, for example). So {root} would get replaced at load time. We might even be able to have that replacement run only at Load time and not have to do it individually for the CFlags and LDFlags.

@dgryski
Copy link
Member Author

dgryski commented Dec 18, 2021

tinygo test -target=wasm compress/bzip2 seems to be failing because the file stuff seems to be unimplemented. I've added a patch to replace {root} in the emulator line so the test at least runs now (even if it doesn't pass.)

@dgryski dgryski force-pushed the dgryski/emulator-main-dir branch from 8493853 to 39ef4d8 Compare December 18, 2021 19:30
@dgryski
Copy link
Member Author

dgryski commented Dec 20, 2021

This is ready for review now.

@niaow niaow dismissed aykevl’s stale review December 20, 2021 18:12

Requested changes have been made.

@niaow niaow merged commit 9eb1388 into tinygo-org:dev Dec 20, 2021
@aykevl
Copy link
Member

aykevl commented Dec 28, 2021

I was starting to look at compileopts.LoadTarget where we already do a bit of munging (for the asyncify extra files, for example). So {root} would get replaced at load time. We might even be able to have that replacement run only at Load time and not have to do it individually for the CFlags and LDFlags.

Actually, I was thinking we should do it the other way round. I don't think I wrote this down anywhere, but the way I see it is that compileopts.Target is a read-only version of the combined JSON files. We do modify it in defaultTarget but that's essentially generating a similar target struct, just without an underlying JSON file (and is not modified after creation).
So:

  • compileopts.Options: command line options and some related things
  • compileopts.Target: JSON file, or inferred from the current system (read-only)
  • compileopts.Config: combines the two and provides the "true" values in methods, see for example Config.Scheduler() that prefers the command line option, then the one specified in the target JSON file, and falls back to the coroutines scheduler (sidenote: that one should probably be updated).

Anyway, that is the design as I have it in my mind.

@dgryski
Copy link
Member Author

dgryski commented Dec 28, 2021

So, should the logic in this PR be moved to compileopts.Config.Emulator() ?

@aykevl
Copy link
Member

aykevl commented Dec 28, 2021

That would be most consistent with the rest of the code.

@dgryski dgryski deleted the dgryski/emulator-main-dir branch January 21, 2022 18:51
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