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 generateResourceAccessor WASI regression #3819

Merged
merged 3 commits into from
Oct 25, 2021
Merged

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Oct 24, 2021

Motivation:

Changes in #3463 broke resource accessor generation when building WASI/WebAssembly projects.

Modifications:

generateResourceAccessor now checks if destination triple is targeting WASI, and uses old behavior from Swift 5.4 that works on this platform.

Result:

Projects targeting WASI that contain resources no longer fail to build.

@MaxDesiatov MaxDesiatov changed the title Fix generateResourceAccessor regression Fix generateResourceAccessor WASI regression Oct 24, 2021
@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor

Do we need to differentiate on path vs. URL for WASI? I think right now it's easy to think that's the main change between the two code paths, when actually the difference is that we ask running compile vs. runtime.

Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

pending @neonichu point about runtime behavior

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Oct 25, 2021

I do prefer compile-time here for WASI, there's no benefit in evaluating this at runtime, especially as Bundle support in WASI Foundation is partial. We expect all resource paths to evaluate to /\(resourceBundleName)/\(resourcePath), which allows us to pass this path to JS functions like fetch directly, or to <img src= HTML attributes. The resources are loaded from the server, and we can't hardcode the host part in the URL. Making URLs relative by starting them with /\(resourceBundleName) makes it work.

CC @kateinoigakukun

@neonichu
Copy link
Contributor

Sounds reasonable to me. I wasn't suggesting to change the behavior, but to change the code to make more clear what's happening, e.g. by adding some comments, so that the next person won't see this as an opportunity to refactor something :)

@MaxDesiatov
Copy link
Contributor Author

Makes perfect sense, I'm adding a comment rn

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Oct 25, 2021

@neonichu I feel seen

@tomerd tomerd merged commit 5b2165d into main Oct 25, 2021
@MaxDesiatov MaxDesiatov deleted the maxd/bundle-path-fix branch October 25, 2021 20:35
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

3 participants