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

feat: retain cli args when relaunching after update, closes #7402 #7718

Merged
merged 22 commits into from
Jan 31, 2024

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Aug 30, 2023

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@amrbashir amrbashir requested a review from a team as a code owner August 30, 2023 16:57
amrbashir and others added 5 commits September 7, 2023 15:19
Co-authored-by: Lucas Fernandes Nogueira <lucas@tauri.studio>
Co-authored-by: Lucas Fernandes Nogueira <lucas@tauri.studio>
@lucasfernog
Copy link
Member

The argument list had a trailing , (using join() is better) that was being denied by one of the bundle targets (probably wix?). Remember to test it with the updater e2e test: cargo test --test '*' -- --ignored.

@lucasfernog lucasfernog added the security: needs audit This issue/PR needs a security audit label Sep 7, 2023
@amrbashir
Copy link
Member Author

amrbashir commented Sep 7, 2023

the test kept failing on my end because Windows defender kept flagging it as a virus and removing it, so I pushed to see if it works on CI and forgot about it but I did test it manually with another app, however I didn't test after I switched the joining logic.

@amrbashir
Copy link
Member Author

And I had to refrain from using .join because it is not implemented for OsString in 1.61

@lucasfernog
Copy link
Member

This needs to be audited. @tweidinger @chippers can this be exploited? I'll go ahead and do the one change I know you'll request, which is pulling from the cached args instead of reading it at update time.

@lucasfernog
Copy link
Member

@amrbashir can you add "change Env.args to pull from std::env::args_os instead of std::env::args" to the v2 breaking change list? :|

@lucasfernog
Copy link
Member

Start-Process : Missing an argument for parameter 'ArgumentList'. Specify a parameter of type 'System.String[]' and try again.
At line:1 char:276
+ ... projects\tauri\target\debug\app-updater.exe" -ArgumentList
+                                                             ~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (:) [Start-Process], ParameterBindingException
    + FullyQualifiedErrorId : MissingArgument,Microsoft.PowerShell.Commands.StartProcessCommand

can you change the updater to check if the argument list is empty and not add the -ArgumentList thing if so?

@lucasfernog
Copy link
Member

Another crate to downgrade on CI: memchr :(

@lucasfernog lucasfernog merged commit 8ce51ce into 1.x Jan 31, 2024
33 checks passed
@lucasfernog lucasfernog deleted the fix/updater-relaunch-args branch January 31, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: needs audit This issue/PR needs a security audit
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

None yet

2 participants