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: run postinstall script explicitly with bash #4116

Merged
merged 1 commit into from
Sep 8, 2021
Merged

fix: run postinstall script explicitly with bash #4116

merged 1 commit into from
Sep 8, 2021

Conversation

max-hk
Copy link
Contributor

@max-hk max-hk commented Sep 7, 2021

Currently, Windows default script-shell (cmd) fails to run the postinstall script.

This commit fixes the problem by running postinstall.sh explicitly with the default bash executable of the OS.

Related: #1397

Currently, Windows default script-shell (cmd) fails to run the postinstall script. This commit fixes the problem by running postinstall.sh explicitly with the default bash executable of the OS.

Related: #1397
@max-hk max-hk requested a review from a team as a code owner September 7, 2021 19:23
@jsjoeio
Copy link
Contributor

jsjoeio commented Sep 7, 2021

First, thank you @MaxLOh for adding this! 🎉

I think this is a great first step toward improving the support on Windows.

I am not on expert in this realm, but I assume this shouldn't cause any issues on other platforms (macOS/Linux)?

@code-asher I'll let you give the final approval since you have more expertise here.

@jsjoeio
Copy link
Contributor

jsjoeio commented Sep 7, 2021

side note: don't worry about the docs preview failing, we need to look into that working on forks. See #4027

@codecov
Copy link

codecov bot commented Sep 7, 2021

Codecov Report

Merging #4116 (3762c39) into main (67b23aa) will not change coverage.
The diff coverage is n/a.

❗ Current head 3762c39 differs from pull request most recent head 06aa785. Consider uploading reports for the commit 06aa785 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4116   +/-   ##
=======================================
  Coverage   64.12%   64.12%           
=======================================
  Files          36       36           
  Lines        1873     1873           
  Branches      379      379           
=======================================
  Hits         1201     1201           
  Misses        571      571           
  Partials      101      101           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67b23aa...06aa785. Read the comment docs.

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Interesting, I guess cmd must not take hashbangs into account. This change does make my senses tingle but I think it should be OK.

Long-term maybe we should rewrite this script to use Node.

@code-asher code-asher merged commit b32b4ed into coder:main Sep 8, 2021
@jsjoeio
Copy link
Contributor

jsjoeio commented Sep 8, 2021

Long-term maybe we should rewrite this script to use Node.

You had me at Node 😍 But agreed, then we could write tests for it more easily

@max-hk
Copy link
Contributor Author

max-hk commented Sep 9, 2021

@code-asher cmd does not recognize forward slashes. It tries to run . instead

C:\>./postinstall.sh
'.' is not recognized as an internal or external command,
operable program or batch file.

If we replace forward slashes with backslashes, Windows will open the .sh file with the associated application, which maybe a random text editor, if not bash.

C:\>.\postinstall.sh

To run the .sh file with the default bash command executable, we will need to run it explicitly with bash

C:\>bash ./postinstall.sh

GirlBossRush added a commit that referenced this pull request Sep 9, 2021
@max-hk max-hk deleted the patch-2 branch September 9, 2021 17:43
@code-asher
Copy link
Member

code-asher commented Sep 9, 2021 via email

jsjoeio added a commit that referenced this pull request Sep 23, 2021
This reverts commit b32b4ed.

We are reverting this because we found out that while this fixes the postinstall
on Windows, it breaks it on mac and other devices.  See:
#3874 (comment)
jsjoeio added a commit that referenced this pull request Sep 23, 2021
Revert "fix: run postinstall script explicitly with bash (#4116)"
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.

3 participants