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

Handle path with whitespaces in build cmd #11692

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

callingmedic911
Copy link
Member

@callingmedic911 callingmedic911 commented Oct 10, 2024

Closes #11464.

  • We could have either quoted the path so it's properly passed as an argument OR we don't use shell: true and let execa deal with it on its own. I went with the latter.
  • I realized we probably don't need shell: true at other places in this file. (we probably don't need shell: true in other commands too, but trying not to creep up the scope of this PR)

@callingmedic911 callingmedic911 added the release:fix This PR is a fix label Oct 10, 2024
@callingmedic911 callingmedic911 added this to the next-release-patch milestone Oct 10, 2024
@callingmedic911 callingmedic911 self-assigned this Oct 10, 2024
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM left a comment

Choose a reason for hiding this comment

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

Looks good. CI is green and I tested locally on mac and windows and it worked well.

I can't recall why we had shell in all the various places we have. I'm sure there were good reasons for some but I bet some were just carried over without too much scrutiny.

@callingmedic911 callingmedic911 marked this pull request as draft October 14, 2024 17:18
@callingmedic911
Copy link
Member Author

I think CI only runs the build without prerender and I made changes to the prerendering part. I still have to followup but Tobbe mentioned a possible case where it could fail https://redwoodjs.slack.com/archives/C03EHC69MFW/p1728584859571649?thread_ts=1728531967.903949&cid=C03EHC69MFW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[Bug]: yarn rw build does not work when the app is stored on a file path with a space character.
2 participants