[GitHub] [lucene-solr] zacharymorn commented on pull request #2068: LUCENE-8982: Separate out native code to another module to allow cpp build with gradle
zacharymorn commented on pull request #2068: URL: https://github.com/apache/lucene-solr/pull/2068#issuecomment-723700269 > I added some general comments. The code, as it is now, doesn't compile for me on Windows (fails to find includes as they're under win32). Even if I add the includes, it fails on compiling the source code. I think the whole native compilation thing should be ignored on Windows as it's not going to work now. Thanks for the comments! I think the reason of failure even with /win32 inclusion is that the native module currently only includes `NativePoxisUtil.cpp`, which seems to be for unix only. The windows equivalent`WindowsDirectory.cpp` still sits in `misc` module and hasn't been moved over yet. From where we are now with the native module, in order to support Windows, we probably just need to add some logic in `build.gradle` to pick the right source and include folders, as well as passing in custom compiler flags. But yes I also feel that this can be tackled in a later stage after the changes for unix are in good shape. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2068: LUCENE-8982: Separate out native code to another module to allow cpp build with gradle
zacharymorn commented on pull request #2068: URL: https://github.com/apache/lucene-solr/pull/2068#issuecomment-724434703 > Zach I've cleaned up the native build a bit - moved it under lucene/misc, added Windows build (it does build the native library for me). I didn't check whether it works on a Mac but I suspect it should. > > I also left the native project included by default in settings (removed the "optional" flag). Gradle's cpp plugin ignores the project on platforms not explicitly mentioned in the targetMachines - I am curious whether we'll blow up something or if it's just going to work. > > While I don't particularly like having native code in Lucene, I think it's better than it used to be (mixed cpp code with java code, etc.). > > I allowed myself to commit directly to your fork, hope you don't mind (please test it out!). Thanks Dawid for the changes! I tested it out in my mac and it built fine as well. I originally separated out into an independent native module thinking that it could host future native code as well, but I guess that's probably just pre-mature optimization as it hasn't been the case for the last few years. > I am curious whether we’ll blow up something or if it’s just going to work. Just curious, are there any cross-platform tests as well in the pipeline that can confirm this? How do we verify this other than running local builds? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2068: LUCENE-8982: Separate out native code to another module to allow cpp build with gradle
zacharymorn commented on pull request #2068: URL: https://github.com/apache/lucene-solr/pull/2068#issuecomment-725807511 > Added the kill switch, Zach. I think you need to merge with master and then update the overview file that Tomoko moved to package-info.java (grep for "build-native-unix" and you'll see old instructions currently in overview.html). Updated. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2068: LUCENE-8982: Separate out native code to another module to allow cpp build with gradle
zacharymorn commented on pull request #2068: URL: https://github.com/apache/lucene-solr/pull/2068#issuecomment-725853994 > All of it looks good, Zach. Sorry about my lack of consistency here but on as second thought I think we should add a safety switch of perhaps force-disabling the native build - just in case we commit it in and have to do it for some reason. > > I'll work on this later today and commit it in. > > I also feel tempted to add tests that utilize this native code. This can come as a separate issue though. I just pushed up a commit that adds tests to exercise the native library loading part. Seems to be working fine on my Mac. Could you please give it a try to see it works on Windows as well? On the other hand, I think these tests will break if run from IDEs. Do we need to support that in this PR? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2068: LUCENE-8982: Separate out native code to another module to allow cpp build with gradle
zacharymorn commented on pull request #2068: URL: https://github.com/apache/lucene-solr/pull/2068#issuecomment-726467539 > > On the other hand, I think these tests will break if run from IDEs. Do we need to support that in this PR? > > Oh, thanks! I'll take a look after I come back from work. I think the PR should be clean in that it doesn't break other people's workflow, so yes - if you added tests they should run or be quietly ignored if they're not supported. I'll take a look. Agreed. I just looked around as well, and it seems @Nightly annotation might be a possible work around here to disable running these tests from IDE. Granted it's a bit of a hack as it also disabled running those tests from developer's local gradle build by default, but at least these tests will get run in nightly build and bugs can be caught (albeit a bit late). Another solution is perhaps to create a new annotation to disable tests running from IDE, if such a thing can be detected ? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2068: LUCENE-8982: Separate out native code to another module to allow cpp build with gradle
zacharymorn commented on pull request #2068: URL: https://github.com/apache/lucene-solr/pull/2068#issuecomment-726643548 > Thanks Zach. I looked at what you did with tests and I think this can be done in a cleaner way that always works. I'll show you how, give me some time. Sounds good! Look forward to it. > Also, update your IDE"s formatter settings to the convention used throughout the code (you used 4 spaces indentation). May be helpful in other patches too. Ops sorry. Just updated it to use 2 spaces indentation. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2068: LUCENE-8982: Separate out native code to another module to allow cpp build with gradle
zacharymorn commented on pull request #2068: URL: https://github.com/apache/lucene-solr/pull/2068#issuecomment-727123057 > No worries. I've pushed the code the way I think it should work - please take a look, let me know if you don't understand something. I tested on Windows and Linux, runs fine. The 'tests.native' flag is set automatically depending on 'build.native' but is there separately just in case somebody wished to manually enable those tests from IDE level. Wow these look pretty advanced! Pretty sure I can't come up with them myself (and I do have a question that I can't seems to find the answer to readily online). I also like that these tests can be run from dev's local environment as well (and they run fine on my mac) compared to the @Nightly annotation approach. Thanks Dawid! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
