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

[#608] Windows fix for <app> console #626

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

artman41
Copy link

@artman41 artman41 commented Jan 10, 2019

Summary of changes

Fixes #608
Modified console.ps1 to:

  • Execute the application inside the current powershell window
  • Hide the output of the start-job call
  • Close the elixir shell on CTRL-C or other breaking keycode
  • Escape the paths for all the arguments passed to erl

Modified release_rc_win_exec.eex to:

  • Properly escape the %boot_script%

Licensing/Copyright

By submitting this PR, you agree to the following statement, please read before submission!

I certify that I own, and have sufficient rights to contribute, all source code and
related material intended to be compiled or integrated with the source code for Distillery
(the "Contribution"). My Contribution is licensed under the MIT License.

NOTE: If you submit a PR and remove the statement above, your PR will be rejected. For your PR to be
considered, it must contain your agreement to license under the MIT license.


& $werl @argv
start-process "$erl" -ArgumentList "$argv" -Wait -NoNewWindow #execute the application in the current shell window
Copy link
Owner

Choose a reason for hiding this comment

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

My impression was that werl is the only "true" Erlang shell, the command-line version is considerably simpler, missing many of the useful features - has that changed? I would expect someone would want the proper shell when attaching a console in production. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

possibly I'm incorrect but I was under the assumption that the two were identical, in the case that they are not, it may be a good idea to have an optional config flag

$argv = @("-boot", $boot)
$argv += @("-config", $Env:SYS_CONFIG_PATH)
$argv += @("-args_file", $Env:VMARGS_PATH)
$argv = @("-boot", "`"$boot`"")
Copy link
Owner

Choose a reason for hiding this comment

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

Just for my own edification, could you explain the changes here? I'm going to merge them, I'd just like to know what I need to be doing differently or how I should be reasoning about quoting here; I'm working with PS Core for the most part, and I know there are differences with previous versions of PS, but from a maintenance standpoint I can't work with both, so Core is what I've stuck with, do these changes make any assumptions in that regard?

Copy link
Author

@artman41 artman41 Jan 18, 2019

Choose a reason for hiding this comment

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

so, the difference here is that we wrap the $boot in quotes so that paths containing a space work as intended, rather than acting as independent args 👍

we also wrap the other args in quotes just in case

@artman41 artman41 force-pushed the master branch 2 times, most recently from a16629d to 949970a Compare February 15, 2019 00:09
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.

2 participants