Skip to content

Remove 'static requirement on run#3006

Merged
kchibisov merged 3 commits intorust-windowing:masterfrom
kchibisov:run-no-static
Aug 5, 2023
Merged

Remove 'static requirement on run#3006
kchibisov merged 3 commits intorust-windowing:masterfrom
kchibisov:run-no-static

Conversation

@kchibisov
Copy link
Copy Markdown
Member

@kchibisov kchibisov commented Aug 5, 2023

There's no need to force the static on the users, given that internally some backends were not using static in the first place.

  • Tested on all platforms changed
  • [-] Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

notgull
notgull reviewed Aug 5, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@notgull notgull left a comment

Choose a reason for hiding this comment

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

This would work for async-winit. In fact, it would work very well, since we could remove the 'static bound on the future in block_on.

However, it looks like this might be unsound on web to my uninformed eye,

@kchibisov
Copy link
Copy Markdown
Member Author

kchibisov commented Aug 5, 2023

@daxpedda @madsmtm @rib

I guess we can't do that for a reason that some backend no-return basically. However, maybe we should just give up and say that users should use spawn/run on web/ios respectively?

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Aug 5, 2023

I think the macOS backend is currently unsound if 'static is not set, since we may not be clearing the callback consistently.

Though that should be fixable.

Re if the backend doesn't ever return, then not having 'static is still fine.

@kchibisov
Copy link
Copy Markdown
Member Author

kchibisov commented Aug 5, 2023 •
edited
Loading

@madsmtm the thing is that we return, but the API consumes the EventLoop so the user can't rerun things, so EventLoop itself is dropped and users can't create it again.

@daxpedda I think I've missed something? Because it seems like other code is bound to 'static?

@madsmtm I was mostly curios about ios.

@daxpedda
Copy link
Copy Markdown
Member

daxpedda commented Aug 5, 2023

This should work fine for Web.
Rust actually just added support for the exception handling proposal, but only with no_std, which wasm-bindgen doesn't support. But when it actually hits run() on Web will be even weirder then before and actually breaks without 'static. I can add a check to make run() not available if exception handling is enabled.

@daxpedda I think I've missed something? Because it seems like other code is bound to 'static?

I can fix that.

madsmtm
madsmtm reviewed Aug 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand you, but I was trying to say that the change should be sound on iOS (even though it doesn't compile as-is).

@daxpedda
Copy link
Copy Markdown
Member

daxpedda commented Aug 5, 2023

@daxpedda I think I've missed something? Because it seems like other code is bound to 'static?

I can fix that.

Required an unsafe conversion, but works.

@kchibisov
Copy link
Copy Markdown
Member Author

kchibisov commented Aug 5, 2023

Required an unsafe conversion, but works.

Yeah, that's what I was afraid of, so I left it as is, unless a backend maintainer approves.

@kchibisov
Copy link
Copy Markdown
Member Author

kchibisov commented Aug 5, 2023

@madsmtm you've replied about macOS initially though, not ios though. I guess your reply about ios was indirect though.

Does it matter for macOS how you cleanup, if you basically drop the EventLoop, like 'static shouldn't matter once you literally call drop on the entire loop in the end of run invocation?

daxpedda
daxpedda approved these changes Aug 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@daxpedda daxpedda left a comment •
edited
Loading

Choose a reason for hiding this comment

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

For Web this works, but probably not useful and I would argue makes run() more and more problematic.

We can have another discussion about whether we should leave run() on Web or not in a different place and time.

@kchibisov
Copy link
Copy Markdown
Member Author

kchibisov commented Aug 5, 2023

I guess I'll do the beta 2 with it like that, and then we could probably gather some feedback whether we'd really need it to keep that way.

kchibisov and others added 3 commits August 6, 2023 01:10
@kchibisov
Remove 'static requirement on run
7c31659 
There's no need to force the static on the users, given that internally
some backends were not using static in the first place.
@daxpedda @kchibisov
Implement Web
7543ce2
@kchibisov
Add ios
6b3ead1
@kchibisov kchibisov merged commit 8100a6a into rust-windowing:master Aug 5, 2023
@kchibisov kchibisov deleted the run-no-static branch August 5, 2023 21:57
@daxpedda daxpedda mentioned this pull request Dec 24, 2023
Merged
@madsmtm madsmtm mentioned this pull request Mar 17, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madsmtm madsmtm madsmtm left review comments

@notgull notgull notgull left review comments

@daxpedda daxpedda daxpedda approved these changes

@jackpot51 jackpot51 Awaiting requested review from jackpot51

@msiglreith msiglreith Awaiting requested review from msiglreith

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kchibisov @madsmtm @daxpedda @notgull