Fix travis build matrix #230
Conversation
nemanja-boric-sociomantic
requested review from
leandro-lucarella-sociomantic and
joseph-wakeling-sociomantic
We're using F env variable to set the flavour of a build. This needs to be explicitly exported into the running environment.
nemanja-boric-sociomantic
force-pushed
the
production
branch
from
253bd93
to
bdf5b83
Compare
|
I can see that from where the makd is putting the build artifacts (they should be in |
|
Oh, interesting alternative choice -- why the switch to doing things this way? |
OK, fair enough. I guess I'm being paranoid about flags ;-) |
Codecov Report
@@ Coverage Diff @@
## v2.x.x #230 +/- ##
==========================================
- Coverage 68.21% 66.65% -1.56%
==========================================
Files 467 486 +19
Lines 40591 43386 +2795
==========================================
+ Hits 27688 28918 +1230
- Misses 12903 14468 +1565 |
Well, because we've already exporting the |
Makes sense, and your |
|
This fucked up coverage though, I presume because production builds might skip some checks (not contracts, right? We are still compiling with contracts for production right?). I think adding codecov as an "AFTER_SCRIPT" was a mistake, coverage reports should be uploaded for all matrix cells, codecov will merged them, and we care about the global coverage, if we have versions that only run with some DMD version for example, the coverage will be different for different builds. |
This is correct, we're still building with contracts, but
If codecov does merging, that makes sense to me. Alternatively, as a quick fix, we can move |
Yes, I think this what we should do now. When originally adding codecov support I didn't notice it can merge reports from same PR, otherwise I would probably do that from the start. |
|
I agree, but would you agree that I should do that in the separate PR, as it's not related to the omitted |
|
Rename PR to "fix build matrix"? :P |
|
OK, let's try it. |
We should upload the coverage for every item in the build matrix, and codecov will merge them
nemanja-boric-sociomantic
force-pushed
the
production
branch
from
e10a5a5
to
73ee571
Compare
nemanja-boric-sociomantic
changed the title
|
Updated with uploading the coverage for all matrix items. |
This is extremely weird. |
|
I wonder if I should export matrix vars while running |
|
Oh, I get it now - it is because of your earlier change https://github.com/sociomantic-tsunami/ocean/blob/v2.x.x/Build.mak#L10-L13 |
|
Ahh yes. |
AFTER_SCRIPT should be used for running the post scripts in travis, and coverage should be enabled for all matrix items, but not by default on developer's machine.
nemanja-boric-sociomantic
force-pushed
the
production
branch
from
e8c268c
to
1102c2c
Compare
|
damn: |
|
So, coverage reporting is broken in D2 and stomping prevention assert. |
|
Ah, crap, I totally forgot about the issue. I will have a quick look if patching dmd-transitional is feasible. |
|
Fails because of weird @@ -1271,7 +1271,9 @@ unittest
bool startsWith ( T ) ( in T[] arr, in T[] prefix )
{
- return (arr.length >= prefix.length) && (arr[0..prefix.length] == prefix[]);
+ if (arr.length < prefix.length)
+ return false;
+ return arr[0..prefix.length] == prefix[];
}
unittest
@@ -1307,7 +1309,9 @@ unittest
bool endsWith ( T ) ( in T[] arr, in T[] suffix )
{
- return (arr.length >= suffix.length) && (arr[$ - suffix.length .. $] == suffix[]);
+ if (arr.length >= suffix.length)
+ return false;
+ return arr[$ - suffix.length .. $] == suffix[];
} |
|
OK, all green now. Let's see what codecov says. |
|
Hm, better, but it still shows some lines that are not covered despite being covered before (if I read report correctly). This is strange. |
|
To me it doesn't look like it says that. I think it says for most of the files here: https://codecov.io/gh/sociomantic-tsunami/ocean/compare/ed68eae659b6c9b93e13d1463d11c84cce7229fa...a2849583c58575cb38a612444f97404de65fddd6/changes that the files had no coverage (left side of |
|
For example, see (compare can be seen here: https://codecov.io/gh/sociomantic-tsunami/ocean/compare/v2.x.x...a2849583c58575cb38a612444f97404de65fddd6/changes) You'll see that the lines that are now not covered, were not actually included in the report before. |
Oh, I'm already working on a beaver-based solution for this, I would leave this to beaver. See #226. I also fixed the original problem addressed by this PR in beaver. |
|
I would still prefer to merge this as is right now because it provides a useful baseline of expected coverage stats when later switching to beaver. Fine with you? |
|
Abandoning to unblock 2.10 as @leandro-lucarella-sociomantic won't be available and this is not important enough. Will be done via #226 |
|
This PR will remain to shame me for eternity to come |
We're using F env variable to set the flavour of a build. This needs
to be explicitly exported into the running environment.