make -V/--version fast on Windows #8508

czlowieq · 2017-10-22T11:48:44Z

Do not run logstash when checking version on windows

original-brownbear · 2017-10-22T12:14:20Z

Jenkins test this please

czlowieq · 2017-10-22T16:20:54Z

I've seen some windows tests in logstash/qa/scripts/windows/acceptance/logstash_release_acceptance.ps1. Should I put this test(s) in there too? I've noticed that there is no version number in environment variables, which could make it tricky.

*nix version tests use rspec, so there is the const containing the version. But I'm not sure if it is possible to test the batch file like this too.

original-brownbear · 2017-10-22T16:24:18Z

@jakelandis @colinsurprenant I think the above is a question for one of you guys :)

jordansissel · 2017-10-26T18:10:44Z

I would like tests added for this. The proposal is simple (shortcut showing the verson) but the implementation is complex and fragile (nobody on the team is an expert in DOS batch, so this may break without us realizing it).

yaauie · 2017-10-26T18:13:00Z

bin/logstash.bat

+goto end
+
+:version
+set "LOGSTASH_VERSION_FILE1=%LOGSTASH_HOME%/logstash-core/versions-gem-copy.yml"


should this be LS_HOME instead of LOGSTASH_HOME? This is the first reference to LOGSTASH_HOME I see in this file, but LS_HOME is referenced several times.

Thanks. Fixed.

czlowieq · 2017-10-29T16:52:15Z

@jordansissel I totally agree that it needs to be tested. I'm not sure how to do it though. It seems that there are tests in PowerShell script, but there is no version number variable, as stated above. Putting the same logic to extract the version number from the same files as in batch is doable, but I'm not sure if it makes sense.

Unless it should be tested in rspec (not sure if it's suitable for rspec tests).

andrewvc · 2017-11-14T03:17:38Z

@czlowieq I think rspec tests are fine for this case. Have you seen the tests in https://github.com/elastic/logstash/tree/master/qa/scripts/windows ?

andrewvc · 2017-11-14T03:18:29Z

Blah, I misspoke, those are all powershell, but that should be fine :)

czlowieq · 2017-11-14T17:26:02Z

@andrewvc Yeah, seen those. But still, I'm not sure if there is a version number I can easily get in those scripts, so I can compare it with the command result. Any ideas?

colinsurprenant · 2017-11-14T20:28:56Z

what this would require is integration testing and we don't have a good story for that yet on Windows. Personally I am ok if we end up merging this without test IIF it has been throughly manually tested and we have other tests for the versions.yml file & content.

I will go ahead and review this shortly. Let me know if anyone has concerns in moving forward like this.

colinsurprenant · 2017-12-05T19:49:58Z

bin/logstash.bat

+if "%LS_HOME%" == "" (
+	for %%a in ("%~dp0..") do set "LS_HOME=%%~fa"
+)
+


this LS_HOME handling is no required since the first thing this bat file does is to call setup.bat which handle the setup of LS_HOME see

logstash/bin/setup.bat

Lines 5 to 19 in 40827a5

rem ### 1: determine logstash home

rem to do this, we strip from the path until we

rem find bin, and then strip bin (there is an assumption here that there is no

rem nested directory under bin also named bin)

for %%I in (%SCRIPT%) do set LS_HOME=%%~dpI

:ls_home_loop

for %%I in ("%LS_HOME:~1,-1%") do set DIRNAME=%%~nxI

if not "%DIRNAME%" == "bin" (

for %%I in ("%LS_HOME%..") do set LS_HOME=%%~dpfI

goto ls_home_loop

)

for %%I in ("%LS_HOME%..") do set LS_HOME=%%~dpfI

colinsurprenant · 2017-12-05T19:55:05Z

bin/logstash.bat

+		)
+	)
+)	
+echo "logstash !LOGSTASH_VERSION!"



++ this seems consistent with the bash version logic. will manually test shortly. 👍

colinsurprenant · 2017-12-05T20:03:05Z

@czlowieq good stuff. I left a few comments - please check about the LS_HOME handling. Other than that I will perform manual test and we should be good to go.

colinsurprenant · 2018-01-05T15:15:17Z

@czlowieq could you please rebase and resolve conflicts? sorry about that, once done I will prioritize testing so we can go ahead and merge. Thanks.

czlowieq · 2018-01-09T15:09:14Z

@colinsurprenant Should I squash the commits into one or leave them as is?

colinsurprenant · 2018-01-11T20:30:37Z

@czlowieq sure - it will have to be done anyway before merging so ...

elasticmachine · 2018-02-16T08:19:48Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

colinsurprenant · 2018-02-19T16:16:13Z

Jenkins please test this

colinsurprenant · 2018-02-19T17:32:20Z

bin/logstash.bat

+		)
+	)
+)
+echo "logstash !LOGSTASH_VERSION!"


this outputs the following:

C:\Users\colin\dev\logstash (pr/8508) λ bin\logstash -V "logstash 7.0.0-alpha1" C:\Users\colin\dev\logstash (pr/8508) λ bin\logstash --version "logstash 7.0.0-alpha1"

To be consistent with the Linux behaviour we should remote the quotes from the output, it should be

echo logstash !LOGSTASH_VERSION!

colinsurprenant · 2018-02-19T17:34:12Z

@czlowieq while manually testing, I noticed that the version string is quoted which is different from the Linux output. Please correct this, then we should be good.
Thanks for your patience.

ppawlikmb · 2018-02-21T07:42:52Z

@colinsurprenant Thanks. Fixed it.

colinsurprenant · 2018-02-21T16:15:21Z

LGTM - merging!

colinsurprenant · 2018-02-21T16:19:50Z

Merged into master and 6.x
Thanks!

original-brownbear requested review from colinsurprenant and jakelandis October 23, 2017 10:52

original-brownbear added enhancement v6.1.0 v7.0.0-alpha1 labels Oct 23, 2017

yaauie reviewed Oct 26, 2017

View reviewed changes

jakelandis removed their request for review November 6, 2017 15:01

elastic deleted a comment from elasticmachine Nov 14, 2017

andrewvc assigned colinsurprenant Dec 1, 2017

colinsurprenant reviewed Dec 5, 2017

View reviewed changes

jsvd removed the v6.1.0 label Dec 11, 2017

czlowieq force-pushed the version-fast-on-windows branch from 92283e2 to 2d7c4bb Compare February 16, 2018 08:19

colinsurprenant reviewed Feb 19, 2018

View reviewed changes

make -V/--version fast on Windows

9ffc83a

czlowieq force-pushed the version-fast-on-windows branch from 2d7c4bb to 9ffc83a Compare February 21, 2018 07:41

colinsurprenant merged commit 30401e3 into elastic:master Feb 21, 2018

colinsurprenant pushed a commit that referenced this pull request Feb 21, 2018

make -V/--version fast on Windows (#8508)

44dc922

colinsurprenant added the v6.3.0 label Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make -V/--version fast on Windows #8508

make -V/--version fast on Windows #8508

czlowieq commented Oct 22, 2017

original-brownbear commented Oct 22, 2017

czlowieq commented Oct 22, 2017

original-brownbear commented Oct 22, 2017

jordansissel commented Oct 26, 2017

yaauie Oct 26, 2017

czlowieq Nov 15, 2017

czlowieq commented Oct 29, 2017

andrewvc commented Nov 14, 2017

andrewvc commented Nov 14, 2017

czlowieq commented Nov 14, 2017

colinsurprenant commented Nov 14, 2017

colinsurprenant Dec 5, 2017

colinsurprenant Dec 5, 2017

colinsurprenant commented Dec 5, 2017

colinsurprenant commented Jan 5, 2018

czlowieq commented Jan 9, 2018

colinsurprenant commented Jan 11, 2018

elasticmachine commented Feb 16, 2018

colinsurprenant commented Feb 19, 2018

colinsurprenant Feb 19, 2018

colinsurprenant commented Feb 19, 2018

ppawlikmb commented Feb 21, 2018

colinsurprenant commented Feb 21, 2018

colinsurprenant commented Feb 21, 2018

	rem ### 1: determine logstash home

	rem to do this, we strip from the path until we
	rem find bin, and then strip bin (there is an assumption here that there is no
	rem nested directory under bin also named bin)

	for %%I in (%SCRIPT%) do set LS_HOME=%%~dpI

	:ls_home_loop
	for %%I in ("%LS_HOME:~1,-1%") do set DIRNAME=%%~nxI
	if not "%DIRNAME%" == "bin" (
	for %%I in ("%LS_HOME%..") do set LS_HOME=%%~dpfI
	goto ls_home_loop
	)
	for %%I in ("%LS_HOME%..") do set LS_HOME=%%~dpfI

make -V/--version fast on Windows #8508

make -V/--version fast on Windows #8508

Conversation

czlowieq commented Oct 22, 2017

original-brownbear commented Oct 22, 2017

czlowieq commented Oct 22, 2017

original-brownbear commented Oct 22, 2017

jordansissel commented Oct 26, 2017

yaauie Oct 26, 2017

Choose a reason for hiding this comment

czlowieq Nov 15, 2017

Choose a reason for hiding this comment

czlowieq commented Oct 29, 2017

andrewvc commented Nov 14, 2017

andrewvc commented Nov 14, 2017

czlowieq commented Nov 14, 2017

colinsurprenant commented Nov 14, 2017

colinsurprenant Dec 5, 2017

Choose a reason for hiding this comment

colinsurprenant Dec 5, 2017

Choose a reason for hiding this comment

colinsurprenant commented Dec 5, 2017

colinsurprenant commented Jan 5, 2018

czlowieq commented Jan 9, 2018

colinsurprenant commented Jan 11, 2018

elasticmachine commented Feb 16, 2018

colinsurprenant commented Feb 19, 2018

colinsurprenant Feb 19, 2018

Choose a reason for hiding this comment

colinsurprenant commented Feb 19, 2018

ppawlikmb commented Feb 21, 2018

colinsurprenant commented Feb 21, 2018

colinsurprenant commented Feb 21, 2018