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: Make CMD handle quotes " properly. #3334

Merged
merged 2 commits into from
Feb 6, 2022
Merged

fix: Make CMD handle quotes " properly. #3334

merged 2 commits into from
Feb 6, 2022

Conversation

philipborg
Copy link
Contributor

@philipborg philipborg commented Feb 5, 2022

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

Technically this is a breaking change, as any workarounds to this bug people have done may stop working. But this would just mean that they can remove those workarounds. But seeing as it hasn't been reported previously and only happens in complex commands I believe it's reasonably safe to make this bugfix. The presumably by far most common workaround, extracting the logic to a script, will likely work without issue.

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

Currently beforeDevCommand and beforeBuildCommand have an issue where they can't deal with quotes properly on windows. Simple example is setting beforeBuildCommand to ECHO %CD% && mkdir \"src-tauri\\websrc\" which will first echo out the expected project repository, then wrongly create the directory C:\src-tauri\websrc.

From CMD /?

If /C or /K is specified, then the remainder of the command line after
the switch is processed as a command line, where the following logic is
used to process quote (") characters:

    1.  If all of the following conditions are met, then quote characters
        on the command line are preserved:

        - no /S switch
        - exactly two quote characters
        - no special characters between the two quote characters,
          where special is one of: &<>()@^|
        - there are one or more whitespace characters between the
          two quote characters
        - the string between the two quote characters is the name
          of an executable file.

    2.  Otherwise, old behavior is to see if the first character is
        a quote character and if so, strip the leading character and
        remove the last quote character on the command line, preserving
        any text after the last quote character.

We just wish to run the command provided in the configuration without extra parsing. Without /S the user must escape certain characters for no benefit as we just wish to run whatever is entered in the configuration.

Sidenote: It is not clear to me by reading the contributing guidelines if bugfixes requires issues to be created. If so I will create one.

@philipborg philipborg requested a review from a team February 5, 2022 18:42
@philipborg philipborg requested a review from a team as a code owner February 5, 2022 18:53
Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

The ",on Windows," is kinda in a weird place (maybe it'd be better at the end?). Anywayyy, good enough for me lol

@FabianLars
Copy link
Member

Urgh, forgot about that. We require all commits to be (gpg) signed.

@philipborg
Copy link
Contributor Author

Urgh, forgot about that. We require all commits to be (gpg) signed.

I will setup signing and rebase my commits then. May tweak the patchnote slightly then as well.

@philipborg
Copy link
Contributor Author

@FabianLars I have now signed my commits. (I think, haven't worked a lot with signed commits before.) I also tweaked the patchnote slightly.

Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you :)

philipborg added a commit to philipborg/CubeShuffle that referenced this pull request Feb 5, 2022
philipborg added a commit to philipborg/CubeShuffle that referenced this pull request Feb 5, 2022
Co-authored-by: Amr Bashir <amr.bashir2015@gmail.com>
@lucasfernog lucasfernog merged commit 52e9a6d into tauri-apps:next Feb 6, 2022
philipborg added a commit to philipborg/CubeShuffle that referenced this pull request Feb 25, 2022
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.

4 participants