-
Notifications
You must be signed in to change notification settings - Fork 231
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
Adding powershell start scripts for windows, fix nodetool ping output #796
Conversation
b60b95d
to
042cd82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank you! I had a couple questions and possible changes.
And preferably the github actions windows tests could be added on to actually build a release with the powershell script and start it, ping it and stop it. Similar to what we now do in shelltests/
.
'Invoke' = 'Uninstall'; | ||
'PassArgs' = $true | ||
}; | ||
'start' = @{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the changes to the other script I think this should now be daemon
. Unless that makes less sense on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equivalent of a "daemon" on windows is a "windows service":
https://en.wikipedia.org/wiki/Daemon_(computing)#Terminology
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added 'daemon' as an alias for 'start'.
priv/templates/nodetool
Outdated
@@ -86,6 +86,22 @@ main(Args) -> | |||
io:format("RPC to ~p failed: ~p~n", [TargetNode, Reason]), | |||
halt(1) | |||
end; | |||
["ping"] -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add these to nodetool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also fixed "nodetool ping ..etc.." which was performing the ping correctly, but then outputting:
Other: ["ping"]
Usage: nodetool {ping|stop|restart|reboot|rpc|rpcterms|eval [Terms]} [RPC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shouldn't be the case, something else is wrong if you are seeing the usage. The extended start script uses nodetool rpc
to do all of the commands, so usage should not be printed because commands like nodetool ping
should never be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current extended_bin_windows is calling nodetool ping:
https://github.com/erlware/relx/blob/4.0.0/priv/templates/extended_bin_windows#L323
I can update these to use rpc instead if you would like?
But.. surely "nodetool ping" should be useable too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, woops. I thought I did updates to the windows script but I must have missed some stuff. You can see in the extended_bin
script that those commands have been replaced by just rpc
because on OTP-23+ we'll be using erl_call
instead of nodetool
and eventually nodetool will be removed https://github.com/erlware/relx/blob/4.0.0/priv/templates/extended_bin#L240
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no more nodetool ping, only rpc - added eval support too
I will have a look at shelltests/ and see what I can add :) |
This looks complicated. I think it's going to require a new github workflow, wintests folder, powershell script and so on, right? |
@reganheath I was thinking just a github action that ran under windows, built a powershell release (simply add an example release to the relx repo, could even be under We can worry about that later if its too much, I wasn't sure how github action windows works anyway and if it gives you powershell by default and lets you just run commands like that or not. |
95b7667
to
6738715
Compare
Well.. that was frustrating! I have implemented a test. I had to implement my own erlang installation process - as the existing action only supports linux (I may contribute my implementation to that project later). The test even found a few bugs, now fixed, and it even passes with OTP 20 and 22, however.. With OTP 19 - relx fails to build:
With OTP 21 - nodetool fails to find epmd.exe Debug Output / Error:
With OTP 23 - I have a bug running erlcall to continue debugging
|
Dang that's dedication, very appreciated. Nice find on the bug. |
Really great! My first guess on the |
In the rebar3 github actions I just use |
Whoa, that build failure is definitely a real issue:
I have no idea how this was only caught by otp-19 on windows... |
@reganheath do you mind just ripping out that first clause, as in remove these two lines in your PR:
|
I've never used it - I'll give it a go. |
Sure. |
OTP bug introduced in 20 perhaps? |
I think this will install Erlang 22.3 and not OTP 19, 20, 21, etc as required by the matrix. |
list -> string Co-authored-by: Tristan Sloughter <[email protected]>
46db569
to
769f427
Compare
0723989
to
33d46f8
Compare
Turns out the OTP 21 issue is a regression in that version, see: So, we cannot test on this version |
33d46f8
to
a6803e1
Compare
You were correct :) |
Question see wintest.yml, what do we want to do in this MR for the install_erlang step. I have provided 3 options, 2 of which will work now and the 3rd being the correct version once my MR for gleam-lang/setup-erlang is accepted. Update my pull request has been merged to gleam-lang/setup-erlang so our path is clear :) |
Update to merged gleam-lang/[email protected]
No longer need local erlang install script
@reganheath awesome work! Is this ready to merge now? |
Yeah that's really awesome work. It's good to see that part of the code get some real love instead of bare-minimum maintenance :) |
Awesome, thanks guys! |
I have created powershell versions of the
bin_windows
andextended_bin_windows
start scripts calledbin_windows_ps
andextended_bin_windows_ps
plus included a psutil utility script.To enable use
{start_script_type, powershell}
in rebar.config relx section. Works with{extended_start_script, true}
and{extended_start_script, false}
Resolves #657 by doing env replacement on .src and handling RELX_REPLACE_OS_VARS.
Resolves #470 by NOT writing erl.ini and instead setting the ERL_LIBS environment variable.
The extension support in extended_bin_windows was not ideal, these implement it fully (as in extended_bin).
I have also fixed "nodetool ping ..etc.." which was performing the ping correctly, but then outputting:
as if it didn't understand the ping command.
I have NOT added any common tests for this. 3 tests appear to be failing, but seem unrelated to my changes - rather I suspect they haven't been run on windows in a while?
I also have not built a release from this and worked out how to include it in rebar3 for a full test.
I have tested the powershell scripts by including them in a release, however I have no idea how to use upgrade/downgrade (relup) so I have not tested it but simply implemented what was already in the existing scripts.
Note, this was based on the 4.0.0 branch.