Travis/fix CI build jobs #206
Conversation
|
ping @Abscissa I don't really know how to review this, but the fact that it passes checks looks promising. |
Maybe I’m missing something, but my undertanding of travis build stages is that all the jobs in one stage must run to completion (AND succeed) before any jobs in the next stage can start. This behavior, AIUI, is the very point of stages, which makes them perfectly suited to things like doing deployment via travis. Unfortunately, that same behavior also makes them poorly-suited to grouping testing jobs into labeled categories as you’re doing here because: A. Test errors/failures in an early job will prevent us from seeing the overall pattern of failures (since later jobs will be skipped), and B. It increases the overall build time for all jobs (since the number of simultaneous jobs running in the pipeline drops below maximum near the end of each stage). However, I do very much like the idea of grouping testing jobs into labeled categories. I think it would make a great feature request for travis, and I’d be in favor of using it here. Unfortunately, unless I’m missing something, travis stages just aren’t quite the right fit for this. What I’ve been doing in lieu of that is to increasingly utilize use the icons and envvars travis displays for each job to help clarify intent as best as possible.
This is a travis bug which currenty has an open PR fix. (Aside: I really wish OS devs - and users – would knock it off with the version nickname BS. I had to lookup
My philpsophy here (and I should probably echo all this into a note in the .travis.yml) is that comprehensive testing and (relatively) broad compatibility is more important than having minor conveniences on my side of things. Doubly so considering this Is a database lib. But keep in mind there are already concessions being made for the sake of build times: I’d love to be able to test this on every combination of supported compiler, supported vibe-d version (including the option of no-vibe), supported DB server and supported platform. (This would also be great because it would simplify the .travis.yml). But that’s obviously a combinatoral explosion and likely abuse of travis’s free infrastructure, so I’ve reined it in and focus instead on the axes (ugh, ie., plural of “axis”) and extremities: Each supported compiler version, and some cherry-picked samples on the other axes, minus the various combinations. But there are still other things that can be done to improve travis build times. Note that each individual job already takes a rediculous amout of time. Something like 1.5 - 2min, IIRC. (And that’s after I already reduced the phobos-socket tests to core-functionality-only and only do the rest of the high-level tests with vibe – though the full testsuite can still be run on phobos outside of travis). A big part of that significant time is because every unittest opens (and closes) a brand-new connection. So we could probably see some very nice testing speedups by using a connection pool for most of those. The difficulty here is this would increase the importance of really, really good unittesting of the connection pool itself, and possibly intoduce some reproducability difficulties when connection pools go wrong. But an additional upside is it would help stress-test the connection pools, which frankly, are what most applications should really be using anyway. I think I would want to rig this as an optional param to the testrunner which could be turned off as a sanity check in case of weirdness (much like how running the tests right now have options of “phobos or vibe sockets” and “core-tests-only or full-suite”). Another angle would be to see if adding the feature of using pipes for local connections instead of sockets could speed up the tests. Overall, there are some things in this PR I like and want to keep, but there are also some issues, and I think overall there’s too much extra stuff in this PR beyond simply fixing the existing travis failures. I’ve addressed some of these details above, and will provide the rest via line notes… |
| @@ -1,21 +1,69 @@ | |||
| language: d | |||
| sudo: false | |||
There was a problem hiding this comment.
I'm unclear as to why this line is removed. Does the osx brewfile stuff require this? If so, then this change is fine.
There was a problem hiding this comment.
This was only removed as it wasn't needed
| @@ -1,21 +1,69 @@ | |||
| language: d | |||
| sudo: false | |||
|
|
|||
| # seems the default 'xenial' isn't working well with mariadb (at some point we need to update to bionic) | |||
There was a problem hiding this comment.
Minor, but I'd alter this comment to clarify that this is a travis bug we're waiting on a fix for:
Ticket: https://travis-ci.community/t/failures-installing-mariadb-10-2-on-xenial/5284/3
PR: travis-ci/travis-cookbooks#1057
There was a problem hiding this comment.
Yes it's due to Travis bug and also the fact that their Ubuntu 18.04 (Bionic) image doesn't support D yet. I'll try find the link for that.
| sudo: false | ||
|
|
||
| # seems the default 'xenial' isn't working well with mariadb (at some point we need to update to bionic) | ||
| dist: precise |
There was a problem hiding this comment.
Yes! Keep this. I figured doing this would probably be simple but hadn't had a chance to look it up.
| include: | ||
| - stage: Test with all dependencies updated (dub.selections.json is deliberately kept old) | ||
| d: dmd |
There was a problem hiding this comment.
I should document this, but the omission of "latest version" tags like "dmd", "dmd-beta", "ldc" is intentional because it destroys reproducability of builds since these labels deliberately change meaning over time. In general I regard them as poor, hacky substitutes for what we REALLY need: a bot that submits PRs for each new compiler version (this would NOT break the concept of reproducible builds).
However, I am in favor of adding jobs for "dmd", "dmd-beta", "ldc", etc as long as they're marked "allowed-failure" (which makes them purely informative). That would definitely be a worthwhile enhancement.
There was a problem hiding this comment.
I don't think latest stable DMD and LDC should allow failures. Dmd-beta is fair enough. The fact that I made this the first stage was so that the build can fail fast if not working with current DMD. That should be the priority
There was a problem hiding this comment.
I think Nick is right, we should have a "failure allowed" version. There is no reason to prevent all development on this project while bugs are fixed in the compiler/phobos. It then becomes a "drop everything" situation. When new versions are released, if the job just works, the new compiler version can be added to the list (if needed).
But I do think we should have something that runs the latest and allows failure, because it would be good to have that warning that something isn't working.
There was a problem hiding this comment.
that's the purpose of having dmd-beta and ldc-beta though. That's where you should allow failures and create a bug ticket if compiler changes are going to break things. When the compiler is released is too late. If dmd and ldc stable are allowed to fail then what would be the point of building with dmd-beta at all?
Also, unless the CI is maintained it will again fall in to the same situation as your master branch in which the latest compiler being tested is dmd-2.080.0, that's a year old! The latest stable is 2.088.0 so in the meantime your codebase could've been broken on any compiler release in that time.
There was a problem hiding this comment.
The issue is simply that you are then allowing an external project to break development on this project. Why should any breaking changes (or bugs in the release that didn't get caught during beta) make it so PRs here cannot pass? In addition, even if you DO find a bug in beta, there is no guarantee it gets fixed before released to stable.
There was a problem hiding this comment.
If the codebase stops working with the latest stable D compiler it's broken anyway and it should fail CI. Allowing the build to continue passing won't resolve anything, it just makes the situation worse. Besides we're not talking an external dependency on some random github code, it's the language itself.
Since this pull request was opened there's already been another dmd release. If I hard code dmd 2.088.0 to be the most recent in the travis file your codebase could be completely broken by language changes in the future and there would be no failing build to flag the problem. Sure you'd have green builds but so what? Better to have a master branch that compiles with the current version of the language.
There was a problem hiding this comment.
If a D release breaks mysql-native's master branch because of legitimate changes, we need a new release to fix that, and that work can happen in parallel with existing work that's being done. The issue is creating "drop everything" problem that is fully triggered by outside projects. There have been several instances where D incorrectly broke major projects, and Phobos/dmd was changed back to fix the problem, because the release wasn't vetted properly. During that time where the projects are waiting for a point release to fix it, there's no reason to halt all development in the D world.
| - d: dmd-2.080.0 | ||
| env: DUB_SELECT=vibe-0.8.3 # DMDFE 2.079+ doesn't support vibe.d v0.7.32 | ||
| env: USE_UNIT_THREADED=true DUB_SELECT=vibe-0.8.6 # DMDFE 2.079+ doesn't support vibe.d v0.7.32 |
There was a problem hiding this comment.
Was there a specific reason USE_UNIT_THREADED is added here?
(TBH, I don't recall offhand if there was a reason it was omitted in the first place...it's possible it may have been an oversight...or it might have been deliberate, in which case shame on me for failing to document the reason!...)
There was a problem hiding this comment.
From the way the builds were setup before it seemed like that variable should be on all builds where D was greater than a specific version
| - d: dmd-2.079.1 | ||
| env: USE_UNIT_THREADED=true DUB_SELECT=vibe-0.8.3 | ||
| - d: dmd-2.078.3 |
There was a problem hiding this comment.
Why is this being removed? Dropping support for old versions IS allowed, but there must be a specific, compelling reason.
There was a problem hiding this comment.
The dmd-2.078.3 build is in the "Test with vibe-d 0.7.32 and older compiler versions" stage.
The dmd-2.079.1 build is last in the "Test with vibe-d 0.8.6 and various compiler versions" stage
| - d: ldc-0.17.5 | ||
| - d: ldc-0.17.1 | ||
| - d: ldc-0.17.0 | ||
| - d: gdc-6.3.0 | ||
| - d: gdc-4.8.5 | ||
| - d: dmd-beta | ||
| env: DUB_SELECT=vibe-0.8.3 # DMDFE 2.079+ doesn't support vibe.d v0.7.32 |
There was a problem hiding this comment.
Again, dropping legacy support is OK, but I need specific, compelling justification.
There was a problem hiding this comment.
There's plenty of good reasons not to focus too much effort on older compilers but again I may have just moved these. Will double check later, on phone now
There was a problem hiding this comment.
Seems I did drop those LDC versions but there's still dmd builds that should cover the same D front end version
| osx_image: xcode9 # use OSX 10.13 | ||
| osx_image: xcode11 # use OSX 10.14 |
There was a problem hiding this comment.
Contrary to what I've said above, I'm ok with osx version bumps like this (unless anyone has reasonable objection), partly because I know little about osx, but mainly because one thing I DO know about osx is that anything less than the latest is pretty much unusable anyway, just as a key part of the OS's ecosystem and fundamental culture.
There was a problem hiding this comment.
Yeah, I can't imagine not updating my Mac. Then again I always update my Linux box to latest Fedora.
| allow_failures: | ||
| # Doesn't appear to exist on travis: https://github.com/travis-ci/travis-ci/issues/8849 | ||
| - d: gdc-6.3.0 | ||
|
|
||
| # Occasional failures are expected here | ||
| - d: dmd-beta | ||
| env: USE_UNIT_THREADED=true DUB_UPGRADE=true DB=mysql-default |
There was a problem hiding this comment.
dmd-beta shouldn't use anything special here, especially not DUB_UPGRADE, because we're specifically testing the compiler beta itself. Testing new versions of other dependencies should be a separate concern and a separate test.
There was a problem hiding this comment.
The older vibe won't support latest D.
| auto dmdZip = "dmd.2.076.0."~environment["TRAVIS_OS_NAME"]~".zip"; | ||
| spawnShell("wget http://downloads.dlang.org/releases/2017/"~dmdZip).wait; | ||
| auto dmdVersion = "2.088.0"; | ||
| auto dmdZip = "dmd."~dmdVersion~"."~environment["TRAVIS_OS_NAME"]~".zip"; | ||
| writefln("Downloading %s from downloads.dlang.org", dmdZip); | ||
| spawnShell("wget http://downloads.dlang.org/releases/2.x/"~dmdVersion~"/"~dmdZip).wait; |
There was a problem hiding this comment.
This is ok, but again, unless this is directly integral to to fixing a failure, this sort of thing really should be a separate PR. And adding additional logging is yet another separate concern from bumping internal versions (which, in turn is also separate from addressing the test failures.)
There was a problem hiding this comment.
If it was my repo I'd have made it another commit. This is, the build takes best part of an hour and I see it as part of the overall job of getting the builds sorted out
| copyIfExists("dub.selections."~envGet("DUB_SELECT")~".json", "dub.selections.json"); | ||
| copyIfExists("examples/homePage/dub.selections."~envGet("DUB_SELECT")~".json", "examples/homePage/dub.selections.json"); | ||
| if(environment.get("DUB_SELECT") != null) { | ||
| string dubSelections = "dub.selections."~envGet("DUB_SELECT")~".json"; | ||
| writefln("Using alternative dub dependencies file: %s", dubSelections); | ||
| copyIfExists(dubSelections, "dub.selections.json"); | ||
| copyIfExists("examples/homePage/dub.selections."~envGet("DUB_SELECT")~".json", "examples/homePage/dub.selections.json"); | ||
| } |
There was a problem hiding this comment.
Again, additional logging is a separate matter and should be a separate PR, but more importantly, this whole additional if is superfluous noise due to the use of copyIfExists.
The additional logging here is not a bad idea, but it's probably better done as an internal enhancement to copyIfExists (and in a separate PR).
There was a problem hiding this comment.
It's not superfluous because of the writeln. You can now see in the build output clearly whether or not it attempts to run through those copyifexists functions
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
This was intentionally 0.8.3 rather than the latest (0.8.6). Testing on the newest dependencies is handled by the DUB_UPGRADE=true builds. These special dub.selections files are for testing on older versions.
BUT that said, what I've said before about dmd vs dmd-2.088 is equally applicable to DUB_UPGRADE=true vs dub.selections.vibe-0.8.6.json. Reproducable builds are important, and DUB_UPGRADE=true fails at that exactly the same as dmd and dmd-beta do. So, contrary to what I've done, the DUB_UPGRADE=true build(s) really should be separate allow-failure builds, and the main "test on the latest version" tests should use this dub.selections.vibe-0.8.6.json.
So I'd like to keep this dub.selections.vibe-0.8.6.json, but in addition to dub.selections.vibe-0.8.3.json, rather than in place of it.
There was a problem hiding this comment.
I don't mind reinstating the old one but I strongly believe you should do away with both and set vibe to 0.8.* in the dub file to start with. I don't think it's sensible to support more compilers then Vibe does. Anyone on an ancient DMD version is already covered by your existing releases. Plan for the future because that's when you're next release will be
|
I can undo the Travis stages and go back to one big matrix. Be aware though that part of what you've said about build stages in that they will not progress to next stage unless pass actually has a benefit. It means the build can fail fast and improve the feedback loop. If you can fail compilation on latest D, why would you want to continue to testing various databases? Also, I get that you want to support a plethora of compiler versions. Realistically though there's not enough people working on this project to do that. DMD and vibe releases are far more frequent than mysql-native. You'd be better off trying to keep up. The existing releases cover old d versions, they're not going anywhere and besides stopping support for DMD 2.068* and DMD 2.07* doesn't mean it's necessarily going to break. Just that the focus is shifted towards the more recent compilers. They should be the priority. Let me know. I can make changes today. |
|
I've done another branch which is similar to this but keeps the matrix of builds instead of using stages: https://github.com/SingingBush/mysql-native/tree/travis/update_build_matrix I'm waiting for that build to pass (https://travis-ci.org/SingingBush/mysql-native) and will open a seperate pull request so that you can chose between the two branches. |
|
How we doing on this and/or #207 ? I'm interested in doing some PRs for mysql-native, but need the CI to work. |
|
I had it almost ready (was slow going just because I was still getting my dev environment set up on my new machine), but then got sick and was out of commission for a couple weeks. Trying to get back to it now.
|
|
@schveiguy: Ok, all's good and working in master now. @SingingBush: I incorporated some of your work on this, thanks. The changes are in the changelog, but basically, mysqln master is now officially switched from "vibe-d:core" to "vibe-core", and the munged OSX "fix" has been ripped out. Instead, mysqln considers vibe broken on osx (because it basically is) and it's officially phobos-sockets-only on osx until vibe-core gets osx sorted out. There's also a few other travis/testrunner improvements as well. There's still a little more I'd like to do on this stuff before tagging a new release, but it's all greenlit and merged into master now, so we should be good to go. |
This PR will resolve the travis build issues. There are 49 jobs that run and I've split them into stages. See: https://travis-ci.org/SingingBush/mysql-native
There seems to be an issue running some jobs on xenial but also bionic didn't seem to work so I've gone back to precise. That can only work for so long, sooner or later the builds will need updating.
It would be a lot simpler (and quicker to build) if there weren't so many combinations of compilers and databases. Please reconsider dropping builds for older dmd versions.