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

SOLR-17490: Check for existence of perl executable and skip if it doesnt exist #2753

Merged
merged 3 commits into from
Nov 16, 2024

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Oct 10, 2024

https://issues.apache.org/jira/browse/SOLR-17490

Description

Make the first time developer experience better with Solr. Today, everytime someone clones Solr for the first time, especially on Windows, they get bitten by the need to install perl.. just to product "Changes.html".

Solution

Add a check for the perl executable and log a warning if it isn't there, and continue on your Gradle process...

Tests

Manual

@epugh epugh requested a review from janhoy October 10, 2024 22:35
@epugh
Copy link
Contributor Author

epugh commented Oct 10, 2024

@patrbraun this one is for you!

Copy link

@patrbraun patrbraun left a comment

Choose a reason for hiding this comment

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

It skips the Perl now. Although it also skips and shows the warning even when Perl is installed.

@dsmiley
Copy link
Contributor

dsmiley commented Oct 11, 2024

I so want this; let's just require Java!
But we actually also require "npm" as well, albeit it'll get auto-downloaded. Any way, one thing at a time.

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

I like this. I actually made a stab at porting the perl script with ChatGPT, but it was too large for context window :) Would probably be a good candidate for a groovy script too, or simply a Java class..

gradle/documentation/changes-to-html.gradle Outdated Show resolved Hide resolved
Co-authored-by: Jan Høydahl <jh@cominvent.com>
@epugh
Copy link
Contributor Author

epugh commented Nov 12, 2024

@patrbraun would you mind taking another stab with the latest change?

@epugh epugh requested review from patrbraun and janhoy November 13, 2024 13:11
@patrbraun
Copy link

It's working for me now.

@epugh
Copy link
Contributor Author

epugh commented Nov 16, 2024

No Changes.txt needed, this is not user facing.

@epugh epugh merged commit 586ed00 into apache:main Nov 16, 2024
3 checks passed
epugh added a commit that referenced this pull request Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants