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

all: replace target=wasi with target=wasip1 #4175

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

ydnar
Copy link
Contributor

@ydnar ydnar commented Mar 4, 2024

This eliminates the wasi build tag in favor of GOOS=wasip1, introduced in Go 1.21.

For backwards compatibility, -target=wasi is a synonym for -target=wasip1.

This eliminates the 'wasi' build tag in favor of 'GOOS=wasip1', introduced in Go 1.21.

For backwards compatablity, -target=wasi is a synonym for -target=wasip1.
@ydnar ydnar changed the base branch from release to dev March 4, 2024 18:28
TODO: should Getpagesize on wasip1 just return wasmPageSize?
@aykevl
Copy link
Member

aykevl commented Mar 4, 2024

While I am in favor of the general idea, there is one problem: we already support wasip1 in a different way. This would add two separate ways of using wasip1 which I think is a bad idea. The other lives in the compiler, here (among others): https://github.com/tinygo-org/tinygo/blob/release/compileopts/target.go#L394

I don't have a strong opinion on either way, but I certainly would want to avoid this duplication. Either by always using the definition in target.go (and no separate JSON file), or by loading the JSON file when using GOOS=wasip1 GOARCH=wasi.

@ydnar
Copy link
Contributor Author

ydnar commented Mar 4, 2024

The latter (loading the JSON file for particular GOARCH/GOOS tuples) is an interesting idea.

That would avoid duplication of config between code and the JSON target files. I’m happy to sketch that out.

@ydnar
Copy link
Contributor Author

ydnar commented Mar 4, 2024

While I am in favor of the general idea, there is one problem: we already support wasip1 in a different way. This would add two separate ways of using wasip1 which I think is a bad idea. The other lives in the compiler, here (among others): https://github.com/tinygo-org/tinygo/blob/release/compileopts/target.go#L394

Remove the duplication of config in target.go in favor of loading a target JSON file seems like a good idea, but that’s a bigger change. Separate PR?

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.

Reviewed, looks good to me with some nits.

Remove the duplication of config in target.go in favor of loading a target JSON file seems like a good idea, but that’s a bigger change. Separate PR?

Seems fine by me.

main.go Outdated Show resolved Hide resolved
Comment on lines +400 to 403
// TODO: should this return runtime.wasmPageSize?
func Getpagesize() int {
return libc_getpagesize()
}
Copy link
Member

Choose a reason for hiding this comment

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

The WebAssembly page size is a fixed constant, and unless WebAssembly decides to introduce a major backwards compatibility break it's not going to change either. I don't think we need to define this in a single place and reuse everywhere (and if we needed to, libc getpagesize would probably be the appropriate place anyway).

In any case, I think it's fine (in fact I'd say just use a numeric constant but a call is fine too).

@ydnar
Copy link
Contributor Author

ydnar commented Mar 5, 2024

Done! I’ll see about a second PR to eliminate the hard-coded target options.

@ydnar ydnar requested a review from aykevl March 5, 2024 17:21
@deadprogram
Copy link
Member

@aykevl any other feedback on this PR, or are we good to merge?

@deadprogram
Copy link
Member

Reminder to @aykevl on this PR, please.

@deadprogram
Copy link
Member

Just looked and realized all the feedback was addressed. Now merging, thank you @ydnar and @aykevl for getting this closer to being all sorted out.

@deadprogram deadprogram merged commit 0559504 into tinygo-org:dev Mar 27, 2024
16 checks passed
@deadprogram
Copy link
Member

Done! I’ll see about a second PR to eliminate the hard-coded target options.

@ydnar you should be unblocked for this now. 😸

@aykevl
Copy link
Member

aykevl commented Jun 14, 2024

Remove the duplication of config in target.go in favor of loading a target JSON file seems like a good idea, but that’s a bigger change. Separate PR?

@ydnar is this something you're still going to do?

@ydnar
Copy link
Contributor Author

ydnar commented Jun 14, 2024

Thanks for the nudge. I'll take a look.

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.

3 participants