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

Revert "fix: run postinstall script explicitly with bash (#4116)" #4231

Merged
merged 2 commits into from
Sep 23, 2021

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Sep 23, 2021

Summary

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)

Additional Context

This change came in #4116 came from @MaxLOh with the goal of:

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.

While that fixed the postinstall issue on Windows, it broke it for anyone installing with Homebrew since our formula does not depend on bash. Instead of modifying the formula, we are opting for reverting b32b4ed and using sh instead.

Our reasoning is that if someone on Windows has bash, surely they have sh as well.

Note: we don't know how @MaxLOh was actually installing code-server so we don't have an exact way to test for their situation but this should work. I had also proposed doing this:

"scripts": {
      "postinstall": "[ $(command -v bash) >/dev/null ] && bash ./postinstall.sh || ./postinstall.sh"
    }

but that assumes the person has command installed Windows which may or may not be the case.

We think we should also try to add some tests for this, which we may do in a follow-up PR.

Todos

Fixes #3874
Fixes #4209

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)
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #4231 (8521799) into main (ed09268) will not change coverage.
The diff coverage is n/a.

❗ Current head 8521799 differs from pull request most recent head 014faf5. Consider uploading reports for the commit 014faf5 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4231   +/-   ##
=======================================
  Coverage   65.09%   65.09%           
=======================================
  Files          36       36           
  Lines        1882     1882           
  Branches      380      380           
=======================================
  Hits         1225     1225           
  Misses        559      559           
  Partials       98       98           

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 ed09268...014faf5. Read the comment docs.

@jsjoeio jsjoeio self-assigned this Sep 23, 2021
@jsjoeio jsjoeio added the bug Something isn't working label Sep 23, 2021
@github-actions
Copy link

github-actions bot commented Sep 23, 2021

✨ Coder.com for PR #4231 deployed! It will be updated on every commit.

@jsjoeio jsjoeio marked this pull request as ready for review September 23, 2021 18:59
@jsjoeio jsjoeio requested a review from a team as a code owner September 23, 2021 18:59
@bpmct
Copy link
Member

bpmct commented Sep 23, 2021

has this been tested on mac / windows / linux? if not happy to jump in and do so :)

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Sep 23, 2021

has this been tested on mac / windows / linux? if not happy to jump in and do so :)

Thanks for offering Ben! Sadly, @code-asher came to the realize that this is not easily tested (at least with the way code-server is packaged up for Homebrew).

We know sh works on mac + Linux so no issues there and we assume it should be fine on Windows. I think if it uses WSL then I assume sh exists and it should work.

I think we'll have to merge and then have the community test and report any issues.

@jsjoeio jsjoeio merged commit f3ef414 into main Sep 23, 2021
@jsjoeio jsjoeio deleted the jsjoeio/revert-4116 branch September 23, 2021 19:33
@bpmct bpmct mentioned this pull request Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants