Skip to content

LUCENE-10461: fix windows launch script for luke so that it works with ITs#743

Merged
mocobeta merged 1 commit into
apache:mainfrom
dweiss:LUCENE-10461
Mar 12, 2022
Merged

LUCENE-10461: fix windows launch script for luke so that it works with ITs#743
mocobeta merged 1 commit into
apache:mainfrom
dweiss:LUCENE-10461

Conversation

@dweiss
Copy link
Copy Markdown
Contributor

@dweiss dweiss commented Mar 11, 2022

No description provided.

@dweiss
LUCENE-10461: fix windows launch script for luke so that it works wit…
fc889ed 
…h integration tests AND actual command line. Cmd escaping rules and start command line is absolutely insane.
@dweiss dweiss requested a review from uschindler March 11, 2022 07:58
@dweiss
Copy link
Copy Markdown
Contributor Author

dweiss commented Mar 11, 2022

@uschindler would you have a look as a fellow Windows user? cmd and start are crazy but I verified it works manually and from within the test, so we should be ok this time.

mocobeta
mocobeta approved these changes Mar 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mocobeta mocobeta left a comment

Choose a reason for hiding this comment

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

I'm not familiar with Windows commands so can't review it - I confirmed this works on Windows 10 (including whitespaces in the path) and passes the distribution tests on Windows.

@dweiss
Copy link
Copy Markdown
Contributor Author

dweiss commented Mar 12, 2022

Thanks @mocobeta ! I'll merge and backport this later today.

@mocobeta
Copy link
Copy Markdown
Contributor

mocobeta commented Mar 12, 2022

@dweiss if you don't mind, I can merge and backport it to branch_9x and branch_9_1.

uschindler
uschindler approved these changes Mar 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

Script works. I haven't tested the testing part. If the env var is correctly set, it should work.

@mocobeta
Copy link
Copy Markdown
Contributor

mocobeta commented Mar 12, 2022

Okay, then let me commit this in...

@mocobeta mocobeta merged commit 25c4310 into apache:main Mar 12, 2022
mocobeta pushed a commit that referenced this pull request Mar 12, 2022
@dweiss @mocobeta
LUCENE-10461: fix windows launch script for luke so that it works wit…
a34072c 
…h integration tests AND actual command line. Cmd escaping rules and start command line is absolutely insane. (#743)
mocobeta pushed a commit that referenced this pull request Mar 12, 2022
@dweiss @mocobeta
LUCENE-10461: fix windows launch script for luke so that it works wit…
a796e08 
…h integration tests AND actual command line. Cmd escaping rules and start command line is absolutely insane. (#743)
@uschindler
Copy link
Copy Markdown
Contributor

uschindler commented Mar 12, 2022

Thanks!

@dweiss
Copy link
Copy Markdown
Contributor Author

dweiss commented Mar 12, 2022

Thank you both.

@mocobeta
Copy link
Copy Markdown
Contributor

mocobeta commented Mar 13, 2022

No problem, thanks for taking care of this.
Though I can't review Windows scripts (there are too many pitfalls), feel free to ping me if a manual test or double-check for the start scripts is needed. I'm always keeping major three operating systems (Windows, MacOS, and Linux) because of Luke and other reasons.

@asfimport asfimport mentioned this pull request Mar 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uschindler uschindler uschindler approved these changes

@mocobeta mocobeta mocobeta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dweiss @mocobeta @uschindler