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

LUCENE-10234: Change module prefix to org.apache.* #487

Merged
merged 6 commits into from
Nov 30, 2021

Conversation

dweiss
Copy link
Contributor

@dweiss dweiss commented Nov 29, 2021

No description provided.

gradle/java/jar-manifest.gradle Outdated Show resolved Hide resolved
@uschindler uschindler self-requested a review November 29, 2021 21:27
@uschindler
Copy link
Contributor

I added a change to the CHANGES.txt file to explain that the automatic module names are a preparation for full module system support. This should be added, because we have not fully tested that everything works well with automatic module names.

Also I added a note that the module names should not be considered "stable". OK?

Copy link
Member

@rmuir rmuir left a comment

Choose a reason for hiding this comment

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

looks good to me

@mocobeta
Copy link
Contributor

Also I added a note that the module names should not be considered "stable".

For what it's worth, I don't think this notice is needed. I've seen many small/large projects that have changed artifact names for various reasons; users have dealt with it (I don't much care about the artifacts/modules name is stable or not, I just search for the right artifacts I need on the Maven central and it works). While the frequent naming changes would be confusing, do we need to strictly define stable names somewhere in the future...? - It's not an opinion but my feeling.

@mocobeta
Copy link
Contributor

mocobeta commented Nov 30, 2021

I checked luke.sh and luke.cmd works for me, just in case.
+1.

From my experiences, launch scripts are the ones of the most fragile and often overlooked parts in software projects. I think Windows bundles PowerShell for years which supports POSIX shell scripts, could we discard .cmd somewhere in the near future releases...

@dweiss
Copy link
Contributor Author

dweiss commented Nov 30, 2021

@mocobeta the powershell scripts have much stricter isolation/ security checks and they would have to be signed. I'm not sure how this works but my experience on github actions infrastructure is that if you start messing with powershell, things stop working.

https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.security/set-executionpolicy?view=powershell-7.2

Copy link
Contributor Author

@dweiss dweiss left a comment

Choose a reason for hiding this comment

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

"Pull request authors can't approve their own pull requests".

...but I do.

}

configure(rootProject) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move this snippet to the dedicated jms section of the build (in the PR that adds full module support). It is useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have full module support, the automatic module name attribute in manifest won't be there anymore. We may need other code to display this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I know. It still belongs over there though.

@mocobeta
Copy link
Contributor

the powershell scripts have much stricter isolation/ security checks and they would have to be signed. I'm not sure how this works but my experience on github actions infrastructure is that if you start messing with powershell, things stop working.

ah, okay... anyway thanks for maintaining it correctly.
I don't think we want to repeat the manual tests for the luke start scripts every release or receive bug reports from users right after publishing the artifacts. If we need them for future releases to come, I'd like to seek a way to integrate a sanity check for it into our smoke test or CI.

@dweiss
Copy link
Contributor Author

dweiss commented Nov 30, 2021

I'd like to seek a way to integrate a sanity check for it into our smoke test or CI.

I know how to do it - I am doing it elsewhere. I just need to get to this part. Track that jms pr, please!

@dweiss
Copy link
Contributor Author

dweiss commented Nov 30, 2021

@uschindler Let me know when you think it's ready and I'll merge/ backport.

@uschindler
Copy link
Contributor

I don't think we want to repeat the manual tests for the luke start scripts every release or receive bug reports from users right after publishing the artifacts. If we need them for future releases to come, I'd like to seek a way to integrate a sanity check for it into our smoke test or CI.

This was also always an issue with Solr, which stopped working on Windows or Linux from time to time. Solr's Smoketester checks this (although not the whitespaces), but Solr does not need a GUI. For Luke it is harder, because we can't easily check a GUI. One idea would be to add some command line parameter to Luke's main class like --sanity-check. If you pass this to script it is justpassing to main function which does some tests if all works.

@mocobeta
Copy link
Contributor

mocobeta commented Nov 30, 2021

I know how to do it - I am doing it elsewhere. I just need to get to this part. Track that jms pr, please!

One idea would be to add some command line parameter to Luke's main class like --sanity-check. If you pass this to script it is justpassing to main function which does some tests if all works.

Thanks, I would not like to do further thread hijacking here, but I'd love to continue the conversation on another issue or PR.

@uschindler
Copy link
Contributor

@uschindler Let me know when you think it's ready and I'll merge/ backport.

Let's wait till this evening to get everybody a chance to bring in hisher opinion. Christian Stein from OpenJDK/JUnit/Maven already answered on the issue.

I sent a HEADS UP as suggested by Adrien to the Mailing list yesterday evening.

lucene/CHANGES.txt Outdated Show resolved Hide resolved
@uschindler
Copy link
Contributor

There was positive review, also from Elasticsearch people, on the dev mailing list, so I think we can merge this. Maybe later this evening or tomorrow morning?

So my +1 to merge soon.

@dweiss dweiss merged commit 20cb681 into apache:main Nov 30, 2021
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.

None yet

4 participants