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

Fixes issue 21059: installer fails when piped on Posix. #463

Merged
merged 2 commits into from Jul 21, 2020

Conversation

veelo
Copy link
Contributor

@veelo veelo commented Jul 20, 2020

The assumption that $SHLVL increases when the script is run does not always hold. $SHLVL does not increase if the script is piped into bash, thereby failing to detect Posix. Also, if the default terminal shell is not bash, $SHLVL might be 1 on Posix.

The assumption that $SHLVL increases when the script is run does not always hold. $SHLVL does not increase if the script is piped into bash, thereby failing to detect Posix. If the default terminal shell is not bash, $SHLVL might also be 1 on Posix.
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @veelo! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21059 normal install.sh: posix_terminal returns false on Linux Mint 20

@jmh530
Copy link

jmh530 commented Jul 20, 2020

Thanks for working on this.

Is this tested with Windows Subsystem for Linux (WSL)?

@veelo
Copy link
Contributor Author

veelo commented Jul 20, 2020

Is this tested with Windows Subsystem for Linux (WSL)?

No it is not tested with WSL. The installer is expected to install the Linux version of dmd on WSL, producing Linux executables (that require WSL to run on Windows).

@nexor
Copy link

nexor commented Jul 20, 2020

I've checked it locally and it seems that issue with docker is resolved.
Thank you!

@veelo
Copy link
Contributor Author

veelo commented Jul 21, 2020

Thanks for confirming, and sorry for the breakage!

script/install.sh Outdated Show resolved Hide resolved
# variable is defined to something like "xterm". If run from a Windows
# command prompt through bash.exe from an MSYS installation, TERM keeps
# its default value, which is "cygwin".
if [[ $TERM == "cygwin" ]]; then
false
Copy link
Member

@wilzbach wilzbach Jul 21, 2020

Choose a reason for hiding this comment

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

there are no booleans in bash!

Suggested change
false
return 0

(I'm aware that it works, but it's still a bad style)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you got it backwards. return 0 == true in bash, unlike D. https://stackoverflow.com/questions/23320150/bash-boolean-testing/23324368#23324368. I believe I had it this way in my first iteration and you commented rightfully that it was confusing. Good or bad style, at least this cannot be misunderstood.

Co-authored-by: Sebastian Wilzbach <seb@wilzba.ch>
@wilzbach wilzbach merged commit 2a3abff into dlang:stable Jul 21, 2020
@wilzbach
Copy link
Member

(deployed)

@veelo veelo deleted the issue_21059 branch July 21, 2020 22:54
@veelo
Copy link
Contributor Author

veelo commented Jul 21, 2020

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants