-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Less statics #14668
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
Less statics #14668
Conversation
Package Changes Through 1ee20a9There are 8 changes which include tauri-utils with patch, tauri-build with patch, tauri-cli with patch, tauri-macos-sign with patch, @tauri-apps/cli with patch, tauri-runtime-wry with minor, tauri-runtime with minor, tauri with minor Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
|
Any chance to run CI automatically on my PRs? Would make iteration much faster, I often miss things built only on other platforms. |
That decision will probably take longer to discuss than your pr will be open x) |
ee5a7b4 to
a1b4415
Compare
|
This is RFR |
|
@FabianLars RFR |
|
Fixed next errors |
Yeah i totally get that. GitHub only offers us 2 options: Auto run workflows for everyone or only for those with write permissions. There's no way to give individuals workflow permissions without also giving write access to the repo. All in all very annoying. You could however enable workflow runs in your own fork (free of charge since it's open source) and open a second PR from your branch against your own fork's |
Didn't think of that, I'm not familiar with the workflows. |
|
whoops sorry, didn't mean to click on button... |
|
To add more context to this PR, it's a bit tersely described, there is an overuse of statics in the code base which lead to verbose code patterns like In quantitative terms, this change removes 43% (76/178) of all unwraps in the |
6e3672f to
93401e3
Compare
|
Fixed CI |
|
Sorry about the late reply. I am a bit skeptical about this change since this means we now need to pass those arguments all the way down The dependency is much more clear though, but I honestly wonder if that really makes a difference if we always initialize the config and directories at he beginning of a command |
|
I don't disagree that the amount of arguments passed to some functions is starting to get unwieldy. I think we can do something about it by redrawing the function boundaries but this PR had to make a judgement call whether to stop at a point where it is still reasonably obvious that the behavior hasn't changed or to continue with less conservative changes.
We actually don't, see the I think the code is much more defensive this way, apart from the substantial reduction of unwraps, it also increases the confidence there are no deadlocks. The pattern of having a mutex in a static is always a bit dangerous, because it can always be accessed and locked from anywhere, and it isn't clear if it has been locked in a parent scope. If we refactor to remove the locks it is a static proof we don't have any deadlocks. Other mutexes that are not in a static coupled with callbacks already led to deadlocks before, see #14694 |
|
I am much more convinced now, thanks for the explanation Looking back, I had quite a few times struggling to understand where is a function looking the directories from, this is definitely a good improvement |
Legend-Master
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from an initial look
My monitor's in repair right now and it's really difficult for me to look at the huge diffs, so this may need to wait until then
Also cc @FabianLars @lucasfernog what you guys think of about this change?
Legend-Master
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the code more closely, this is definitely a great improvement to readability, awesome work!
So happy my monitor's finally back today 😁
|
As always, thanks for taking the time to review my PRs, much appreciated. More coming up soon.. |
* refactor(tauri-cli): introduce replacement functions * refactor(tauri-cli): apply replacement to remove.rs * refactor(tauri-cli): apply replacement to icon.rs * refactor(tauri-cli): apply replacement to bundle.rs * refactor(tauri-cli): apply replacement to build.rs * refactor(tauri-cli): apply replacement to add.rs * refactor(tauri-cli): apply replacement to dev.rs * refactor(tauri-cli): less controlflow * refactor(tauri-cli): split config loading from locking static * refactor(tauri-cli): remove duplicate checks covered by if let Some(tauri_dir) = tauri_dir tauri_dir.is_some() must be true, otherwise the entire block is not run, so the frontend_dir check is irrelevant * fmt * refactor(tauri-cli): apply replacement to inspect.rs * refactor(tauri-cli): dont use statics for config * refactor(tauri-cli): finish threading directory paths through functions * fix: clippy * fixup CI * refactor(tauri-cli): dont need mutex * refactor(tauri-cli): rescope mutex use to minimal necessary * fix CI, reduce mutex use * fixup PR tauri-apps#14607 * fix: clippy * refactor(tauri-cli): remove ConfigHandle * refactor(tauri-cli): remove more unwraps and panicing functions * refactor(tauri-cli): less mutexes * refactor(tauri-cli): undo mistaken change, address review comment * Split android build to 2 functions * Pass in dirs to migration v1 like the v2 beta * Add change file --------- Co-authored-by: Tony <[email protected]>
Work on refactoring the globals in tauri-cli. I think this is beneficial as-is,
but it is not quite finished.This still is missing a full transition towards lazy initializedtauri_dirandfrontend_dirwhile guaranteeing not to break anything, as discussed in this comment.Completed.
Read commit-by-commit.