Conversation
| let status = child | ||
| .wait() | ||
| .expect("failed to wait on \"beforeDevCommand\""); | ||
| if !(status.success() || KILL_BEFORE_DEV_FLAG.load(Ordering::Relaxed)) { |
There was a problem hiding this comment.
The use of Ordering::Relaxed is almost always incorrect without use of memory barriers, but most CPUs don't even have real relaxed orderings, so the bug will hardly surface in practice.
Package Changes Through c73682bThere are 9 changes which include tauri with minor, @tauri-apps/cli with minor, tauri-cli with minor, tauri-utils with patch, tauri-build with patch, tauri-macos-sign with patch, tauri-bundler with minor, tauri-runtime-wry with minor, tauri-runtime 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 |
|
I'm not opposed to splitting this PR, but it seems a bit synthetic if it's just done to work well with a squash. That said, I'm not familiar with the benefits of squashing, so interested to hear a different take. |
|
First off, we have set to only allow squash merge (could of course make exceptions) To me with squash merge, we maintain a linear history and can create as many small commits in a PR without having to worry about polluting the main history
I fully understand this though, I just think there're too much unrelated things to the pull request topic, like the entire busy loop fix is ~10 lines in 8634d96 but the refactor of other things took ~200 lines (very much appreciate the clear commit messages though) And also in this specific case, the removal of the |
|
Managed to mess up the change file, rerun.
Yeah, I agree in this specific case, splitting is fairly easy. If I'm already 20 commits in and splitting would be a gnarly rebase, I hope we can be flexible if the need arises :) |
Legend-Master
left a comment
Legend-Master
left a comment
There was a problem hiding this comment.
Would like to wait a bit for the response and see if we can remove manually_killed_process entirely
| @@ -86,7 +81,7 @@ impl DevProcess for DevChild { | |||
| } | |||
|
|
|||
| fn manually_killed_process(&self) -> bool { | |||
There was a problem hiding this comment.
Looking at the code, it seems like the use for this is removed in #11694 and we should be able to remove this entirely right? cc @FabianLars
There was a problem hiding this comment.
I haven't removed it because I may still need it for an upcoming PR that hasn't been started yet.
There was a problem hiding this comment.
Looking at the code, it seems like the use for this is removed in #11694
Hmm yeah, now that you mention it it's not actually used on mobile, i didn't check that in the linked PR and just left mobile untouched.
There was a problem hiding this comment.
I haven't removed it because I may still need it for an upcoming PR that hasn't been started yet.
We can leave it for now then
Co-authored-by: Tony <68118705+Legend-Master@users.noreply.github.com>
3 participants
Make more things concrete rather than abstract.