On Fri, Sep 20, 2019 at 1:37 AM Dawid Weiss <dawid.we...@gmail.com> wrote:
> Hi Mark, > > > I find the Gradle way of doing things odd, but I knew better than to try > and go directly against them. > > So none of this is really my choosing, I went with how you are supposed > to do it. > > Nah -- that's not really right. Gradle is really flexible and there is > no "convention" in my opinion. Everyone seems to do things their own > way. What you mentioned perhaps results from this layering of build > environment property shadowing: > > https://docs.gradle.org/current/userguide/build_environment.html > > What we deal with are two different categories of properties -- > project properties (like tests.seed and other things affecting the > build) and properties configuring gradle itself (like > daemon-disabling, parallelisation, etc.). My concern lies with local > configuration of the latter; messing with global gradle configuration > affects any other project you're working on which is what I'm not > comfortable with. > > There is convention for this :) The convention is to commit your gradle.properties and that is what holds your project config. It's widely done and promoted by the Gradle team. Bucking that is confusing and against best practices. What you do with your global gradle file is up to you, has nothing to do with convention, it just works how it works. If you don't want to put props in that file, no one will make you :) > > When it comes to allowing another prop file just for our project, I'm > all for it - just not the standard gradle.properties that is supposed to be > committed in a gradle project. > > It doesn't have to be committed nor is it needed in most cases. Just > about the only exception is when you need to alter gradle itself -- > increase the heap or force some other settings. Like here: > > https://github.com/elastic/elasticsearch/blob/master/gradle.properties > > For anything else you can use your own properties (read from anywhere > you like) and layer them in any way you like. > > The convention is to commit it with gradle :) This is where the project properties like the version and often dep versions live. If you want a properties file that is not committed, you should make another and thats fine, just like build.properties with ant now. > > You cannot override any settings from your gradle home file regardless, > so it's a little non intuitive - gradle home props rule all. > > Yes and that's exactly my concern. Even when you think of > experimenting on different Lucene branches or having a "pristine" > build -- a clean checkout should just work without you having to worry > about any global overrides (in my opinion). A local non-versioned > developer-only override file would work nicer here. > > > Anyway, I have exposed like one of our properties, so no, it's not > thought out or how it's going to be at all. Probably that property should > be name spaced beyond tests_jvms (translated from tests.jvms), > > They can be dot-separated and this works just fine. You need to be a > bit more careful in groovy to quote it as a property but otherwise > it'll work just fine. See the examples I committed yesterday for > tests, for instance -Ptests.iters=5 > > > Sucking in a user prop file like we do with ant sounds great, just have > to get used to the fact that you cannot override your global settings. > > That's my point -- if we don't have to mess with global settings then > we don't have to worry about it. Your "presets" task would still work > like a charm but instead of modifying globals it'd modify > local-non-versioned override file. This is easy to achieve (but you > have to move them out from gradle.properties which is a > higher-precedence level).]\ > My presets are not how this is supposed to work! They are a simple tool to help non gradle people get something working today (like literally today) with reasonable settings. That is not how I expect normal people to configure gradle. It's def not how I expect someone that knows gradle to work. This is like if I made the ant build and showed you how to set some properties to try it out and you are like, no, I don't want to set properties via command line, that's crazy! I'd be like, okay, don't. I don't care how you want to set properties and I'm not here to set them for you (but I do want to give Gradle noobs that have to switch some easy options to get started!). > > > Anyway, I've never found it to be a problem. I didn't expect everyone > would customize our project settings in their gradle home file, I usually > put stuff there that I want to apply to all my checkouts (tests.jvms is > actually one of them), but all that is very individual. > > I think the configuration we have right now is incorrect in principle > (the two different types of configuration settings I mentioned at the > start are mixed). The daemon in particular already affected me > (because I'd like to tweak build settings with the daemon for faster > turnaround but at the same time not have it overridden globally). > I'm not really sure what you are talking about. The daemon is not controlled by us, it's controlled by you and what you put in your global file. I turned off the daemon in our config so that on CI it won't be used when no setting is in the global config. We can do that other ways. I think you are confused about what I'm doing here. This project is not trying to control your gradle settings, nor does it need to. It's trying to pick good defaults if you have not chosen to make those choices yourself. I don't expect anyone that knows Gradle to use the little task I have that spits out props. Thats also just going to spit them to the screen by default, it's going to be a tool to give noobs an idea of what can be set and what settings might work best for our project. > > > But running tests within a module, or even running all modules tests, > was no good. You have solr-core tests coming late, you have many tests that > pop up from short runs to minutes+ (often tests with many sub class tests), > and you have a static JVM assignment for each test. So basically, it's easy > to be running long tests at the end of a run and use few JVM's, and with > our tests, this can be devastating. > > As for "soft" ordering of test tasks between modules you could try to > configure cross-module task precedence with mustRunAfter (set it up > programmatically)... this is a bit of an overkill but it'll ensure no > tests run before the core tests are finished, for example. > > > We are a terrible case for gradles parallel tests. > > I read the issue history on their tracker. Indeed the situation could > be improved. This is something we can work on later though, really. We > can even switch the tests to an ant runner for now. The benefits of > other cleanups are worth facing the tests problem. > That I would rather not do - I think gradle test should stick to being gradle test, I saw that ES did that and it's something I disliked from the start. > > > So I started with solr-core generally taking 20+ minutes or more on my > 18 core machine, and with some other simple test taming, testFail can now > do it in just under 7 minutes. > > I haven't seen the implementation of this yet, but it's interesting. > My general remark for the gradle build system switch would be that we > try to make it simpler and easier... :) If you keep adding hardcore > tweaks of gradle internal tasks (like you already do) then things will > become hard to follow for others (like me) and you'll remain the only > person who understands how it works. :) > I would say this: their code to manage this is 1000 times more simple than the code to do it in randomized testing :) What are the current hardcore tweaks I'm making by the way? Bring them up for discussion. I think the only one that is really there is the plugin that makes deps actually work nice. Everything else I shoot for convention or best practices. It's just that without the consistent versions plugin, deps across modules in Gradle suck. Beyond that, having a target that works with tests until Gradle is not silly with them is really the only way to fix tests until they do a better job. I've made a second target so that anyone that wants to run tests the slow standard way can. The other Gradles targets will still depend on test. I'm not a fan of replacing internal Gradle tasks. I've been very careful about what I've added or changed actually - it's very well thought out. For something like testFast (or whatever it's called), first it's super simple code, second, it's not something we need to rely on (it's bonus). Third, it's ridiculously better than the test task for running tests in our project. Perfect qualities. -- - Mark http://about.me/markrmiller
