Refactor CI avoiding test circular dependencies #227

vdukhovni · 2020-05-27T19:22:53Z

Instead of running tests in the parent "bytestring" project, with
circular dependencies from the test frameworks, build and run them in
the child bytestring-tests project, which builds the parent as a
dependency that does not run tests, avoiding all the loops.

This requires cabal-install >= 2.4 with cabal.project doing most of the
heavy lifting.

vdukhovni · 2020-05-27T19:26:43Z

@sjakobi @hvr @cartazio @bgamari @Bodigrim
I believe this is ready and makes testing substantially easier going forward.

cartazio · 2020-05-27T20:25:05Z

Looks great to me!

…

On Wed, May 27, 2020 at 3:26 PM Viktor Dukhovni ***@***.***> wrote: @sjakobi <https://github.com/sjakobi> @hvr <https://github.com/hvr> @cartazio <https://github.com/cartazio> @bgamari <https://github.com/bgamari> @Bodigrim <https://github.com/Bodigrim> I believe this is ready and makes testing substantially easier going forward. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#227 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABBQQDOSW5LXA4ULAHWETRTVSQDANCNFSM4NMNYHUQ> .

sjakobi

Sorry for the load of questions. This is clearly a good direction, I'm just homing in on the details. :)

Also feel free to ignore what seems to go too far! :)

I haven't actually tried to use this locally yet. Would you consider adding instructions on how to build an run the tests to the readme? I'd definitely give those a spin! :)

tests/bytestring-tests.cabal

sjakobi · 2020-05-28T00:22:54Z

tests/cabal.project

+package bytesting-tests
+    tests: True


Why not

Suggested change

package bytesting-tests

tests: True

tests: True

?

We don't want to test any of our dependencies. I am not sure what the scope of that is, it may cover dependencies.

The default scope are the packages listed above AFAIK.

cabal.project

sjakobi · 2020-05-28T00:25:12Z

cabal.project

@@ -0,0 +1,5 @@
+packages: bytestring.cabal
+allow-newer: *:bytestring


Surely there can't be any reverse dependencies of bytestring involved here?

When testing with GHC-head, text, et. al. needed for the tests want an older bytestring than the one we're testing, but we want to force them to use the new one.

But this cabal.project doesn't include the tests?!

.travis.yml

sjakobi · 2020-05-28T00:38:36Z

.travis.yml

+ - cabal v2-configure; cabal v2-build
+ - cabal v2-sdist -o .
+ # Remove to enable build of sdist.
+ - rm -f cabal.project
+ - rm -f tests/test-builder.tix
+ - (cd tests; cabal v2-configure; cabal v2-test)
 # "cabal check" disabled due to -O2 warning
- - export SRC_TGZ=$(cabal info . | awk '{print $2 ".tar.gz";exit}') ;
-   cd dist/;
-   if [ -f "$SRC_TGZ" ]; then
-      cabal install --force-reinstalls "$SRC_TGZ";
+ - export SRC=$(cabal info . | awk '{print $2; exit}');
+   if [ -f "$SRC.tar.gz" ]; then
+      cabal get "./$SRC.tar.gz";
+      (cd "$SRC"; cabal install --force-reinstalls .);


Did you consider simply using https://github.com/haskell-CI/haskell-ci to generate this script?

Actually, that might be tricky…

I can give this a spin once this PR has been merged.

I'm having a devil of a time getting it to work portably between Cabal 2.4, 3.0 and 3.2, not sure the autogenerated one would fare better.

Got it to work, but had to resort to v1-install with Cabal 3.x, if you figure out a working, non-deprecated syntax, I'm all ears...

Compatibility issues like that are a haskell-ci specialty. ;)

No worries though. Looks like CI is finally green now!

sjakobi · 2020-05-28T00:40:43Z

tests/cabal.project

@@ -0,0 +1,9 @@
+packages: bytestring-tests.cabal
+          ../bytestring.cabal


Why include the bytestring package here? Doesn't this cause test dependencies to be recompiled after changes to bytestring code?

IIRC the test frameworks and text need to use the same version of ByteString as being tested. We compiling the tests with code that also uses Text, which uses ByteString, ... that cyclic thing. But here we're build the test module, and bytestring is just one of the dependencies to build along with the others.

Give it a try:

cd tests cabal configure cabal test

then tweak cabal.project and try again...

sjakobi · 2020-05-28T08:55:11Z

Many thanks for responding to my comments above, @vdukhovni! I have, in turn, tried to respond to your responses, but I don't have the necessary permissions to "unresolve" discussions. Would you mind unresolving these things for me?

Alternatively, maybe @hvr or @cartazio could give me the necessary permissions. That would be greatly appreciated! :)

vdukhovni · 2020-05-28T22:50:29Z

I've unresolved all the previous comments, which I marked "resolved" primarily so I could see what I had not yet addressed, otherwise hard to find... Sorry...

sjakobi · 2020-05-29T09:56:10Z

I just gave this PR as spin on my local machine. The issue that I believe we're close to eliminating now is that after any change to the bytestring library code, the entire set of test dependencies is rebuilt, which takes a bit of time. A development ergonomics problem, IMHO.

What we need to do is to keep the local bytestring package out of the bytestring-test.

At this point, that's very simple to achieve:

Revert 84f15d5 (I really don't understand where you wanted to go with that @vdukhovni)
Remove the local bytestring package from the test cabal.project:

--- a/tests/cabal.project
+++ b/tests/cabal.project
@@ -1,5 +1,4 @@
 packages: bytestring-tests.cabal
-          ../bytestring.cabal

 allow-newer: *:bytestring
            , *:ghc-prim

Now, rebuilding the tests is super-fast. Great!

sjakobi · 2020-05-29T10:06:28Z

(And thanks for unresolving my earlier comments! It's a bit of a shame that we have to jump through these hoops to collaborate!)

vdukhovni · 2020-05-29T18:12:40Z

I just gave this PR as spin on my local machine. The issue that I believe we're close to eliminating now is that after any change to the bytestring library code, the entire set of test dependencies is rebuilt, which takes a bit of time. A development ergonomics problem, IMHO.

Rebuilding the test dependencies against the updated bytestring is actually necessary. Otherwise the test dependencies are linked against the wrong bytestring. The time to run the tests while making changes to bytestring should not influence the decision much or at all. It is pretty quick anyway.

What we need to do is to keep the local bytestring package out of the bytestring-test.

No, we actually need to make sure that we're testing the local bytestring package and not the stock bytestring from the package cache.

At this point, that's very simple to achieve:

Revert 84f15d5 (I really don't understand where you wanted to go with that @vdukhovni)

Remove the local bytestring package from the test cabal.project:

I disagree. Please try for example building Andrew Martin's PR, or the ryu PR without this. (For the former you'll need GHC master, and Simon PJ's patch), ...

Now, rebuilding the tests is super-fast. Great!

I disagree, and don't find the time to build tests a deciding factor.

sjakobi · 2020-05-29T18:20:07Z

Rebuilding the test dependencies against the updated bytestring is actually necessary. Otherwise the test dependencies are linked against the wrong bytestring.

Ok, I'm curious now. Why ought the test dependencies to be linked against the new/local bytestring?

The time to run the tests while making changes to bytestring should not influence the decision much or at all.

I think it's actually pretty important.

Note that the containers setup takes this idea even further. It's containers-tests sub-package simply has the same library component as containers proper. That way, modules used by multiple testsuites are recompiled only once.

vdukhovni · 2020-05-29T18:24:51Z

Note that the containers setup takes this idea even further. It's containers-tests sub-package simply has the same library component as containers proper. That way, modules used by multiple testsuites are recompiled only once.

Are you reachable via any interactive channels? (Skype, Hangouts, Signal)? Please send me an email [email protected]. For me the test runtime from cold start was:

real    1m57.371s
user    2m44.350s
sys     0m16.233s

Which is not a barrier to running tests now and then... Builds w/o tests take 16s (cold).

sjakobi · 2020-05-29T18:36:32Z

I've sent you an email.

My concern is about interactive development. You're working on some function, and you want to run the tests for that. Recompilation should be as fast as possible then. 1 or even 2 minutes would be very frustrating already.

vdukhovni · 2020-05-29T21:32:17Z

With great help from @sjakobi I've simplified this further, and expect I've addressed his concerns. The CI should shortly show up green. The cabal.project files are now quite short, and we're no longer rebuilding the test dependencies against the new bytestring, just testing bytestring-test as suggested.

The only downside is that modules need to be listed in two .cabal files, bytestring and bytestring-test

sjakobi · 2020-05-29T22:28:23Z

Thanks @vdukhovni. I very much enjoyed our chat! :)

FYI, I believe I've found the discussion on the cabal feature that would make much of this work redundant by allowing circular dependencies in testsuites: haskell/cabal#4087

vdukhovni · 2020-05-29T22:33:14Z

FYI, I believe I've found the discussion on the cabal feature that would make much of this work redundant by allowing circular dependencies in testsuites: haskell/cabal#4087

Yeah, that would have been nice. But it's been under discussion for 3 years, so I'm not expecting a miracle... I've had to just force push a small fix, it seems I needed a v2-build rather than build for testing that that the unpacked sdist is buildable...

Edit: Finally all green again: https://travis-ci.org/github/haskell/bytestring/builds/692741896?utm_source=github_status&utm_medium=notification

Barring any surprises, I'm calling this done. Moving on to better things...

sjakobi

It would be great if someone with better bash skills (EDIT: than mine) could review the Travis script.

For local development, this setup seems quite awesome! :)

Again, many thanks, @vdukhovni!

.travis.yml

vdukhovni · 2020-05-29T22:47:58Z

It would be great if someone with better bash skills could review the Travis script.

I guess, sadly, my own bash power-user status does not count, you're looking for someone else to review...

vdukhovni · 2020-05-29T22:51:27Z

@hvr, @cartazio. At this point I'm looking for a final review and merge, after addressing any issues.

sjakobi · 2020-05-29T22:52:55Z

It would be great if someone with better bash skills could review the Travis script.

I guess, sadly, my own bash power-user status does not count, you're looking for someone else to review...

Indeed. I'm looking for a second pair of eyeballs in addition to yours here. My own count for at most half an eye… ,)

bgamari · 2020-06-20T19:19:36Z

@hvr, @cartazio, @dcoutts, this is in need of review.

Instead of running tests in the parent "bytestring" project, with circular dependencies from the test frameworks, build and run them in the child `bytestring-tests` project, which builds the parent as a dependency that does not run tests, avoiding all the loops. This requires cabal-install >= 2.4 with cabal.project doing most of the heavy lifting.

vdukhovni · 2020-06-25T00:18:16Z

Rebased as mentioned in #214

sjakobi · 2020-07-01T23:18:28Z

@Bodigrim, @hsyl20, could you take a look at this?

I'm not too worried about potential problems with this. If any issue with CI comes up, we can fix it on the go.

If no one points out any problems with this patch, I'd simply merge it in 2 or 3 days.

Bodigrim

Looks good to me.
We will have to switch to 8.8.4 and 8.10.2 anytime soon, but I guess we can do it in a separate PR as well.

hsyl20 · 2020-07-02T07:20:37Z

Looks good to me too.

sjakobi · 2020-07-02T10:14:41Z

Thank you, @vdukhovni! :)

vdukhovni · 2020-07-02T17:28:04Z

.travis.yml

+ - export SRC=$(cabal info . | awk '{print $2; exit}');
+   if [ -f "$SRC.tar.gz" ]; then
+      cabal get "./$SRC.tar.gz";
+      (cd "$SRC"; cabal v2-build);
   else
      echo "expected '$SRC_TGZ' not found";


Oops, the error message here is not quite right, it should be $SRC.tar.gz not $SRC_TGZ which is no longer set. This should not happen, but if it ever did, the error message would be confusing...

@sjakobi Would you care to fix the error message?

I'm not too worried about it. Feel free to send a PR or open an issue.

vdukhovni force-pushed the ci-refactor branch from 06660a0 to dd5aa17 Compare May 28, 2020 00:31

sjakobi reviewed May 28, 2020

View reviewed changes

vdukhovni force-pushed the ci-refactor branch from dd5aa17 to 48c4c09 Compare May 28, 2020 01:09

vdukhovni mentioned this pull request May 28, 2020

Implement floating point conversion with ryu #222

Closed

vdukhovni force-pushed the ci-refactor branch from 5de08b6 to 84f15d5 Compare May 29, 2020 09:04

vdukhovni force-pushed the ci-refactor branch 3 times, most recently from ce572c3 to 29567fe Compare May 29, 2020 20:36

vdukhovni force-pushed the ci-refactor branch from 29567fe to afcdab1 Compare May 29, 2020 22:28

sjakobi approved these changes May 29, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

vdukhovni mentioned this pull request Jun 23, 2020

Would be nice to push a 0.10.10.1 release #214

Closed

vdukhovni force-pushed the ci-refactor branch from afcdab1 to be95a40 Compare June 25, 2020 00:17

vdukhovni mentioned this pull request Jun 25, 2020

Document possible interleaving of hPutStrLn. #201

Closed

sjakobi linked an issue Jun 25, 2020 that may be closed by this pull request

Full rebuild on test modification #120

Closed

Bodigrim approved these changes Jul 1, 2020

View reviewed changes

vdukhovni mentioned this pull request Jul 2, 2020

Time to fork bytestring into a stable legacy and new feature releases unblocking progress. #234

Closed

.travis.yml: Remove unnecessary .tix file deletion step

ddf8d43

sjakobi merged commit 5970194 into haskell:master Jul 2, 2020

vdukhovni commented Jul 2, 2020

View reviewed changes

		@@ -0,0 +1,5 @@
		packages: bytestring.cabal
		allow-newer: *:bytestring

		@@ -0,0 +1,9 @@
		packages: bytestring-tests.cabal
		../bytestring.cabal

Refactor CI avoiding test circular dependencies #227

Refactor CI avoiding test circular dependencies #227

Conversation

vdukhovni commented May 27, 2020

vdukhovni commented May 27, 2020

cartazio commented May 27, 2020 via email

sjakobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjakobi commented May 28, 2020 • edited Loading

vdukhovni commented May 28, 2020

sjakobi commented May 29, 2020

sjakobi commented May 29, 2020

vdukhovni commented May 29, 2020

sjakobi commented May 29, 2020 • edited Loading

vdukhovni commented May 29, 2020

sjakobi commented May 29, 2020

vdukhovni commented May 29, 2020

sjakobi commented May 29, 2020

vdukhovni commented May 29, 2020 • edited Loading

sjakobi left a comment • edited Loading

Choose a reason for hiding this comment

vdukhovni commented May 29, 2020

vdukhovni commented May 29, 2020

sjakobi commented May 29, 2020

bgamari commented Jun 20, 2020

vdukhovni commented Jun 25, 2020

sjakobi commented Jul 1, 2020

Bodigrim left a comment

Choose a reason for hiding this comment

hsyl20 commented Jul 2, 2020

sjakobi commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjakobi commented May 28, 2020 •

edited

Loading

sjakobi commented May 29, 2020 •

edited

Loading

vdukhovni commented May 29, 2020 •

edited

Loading

sjakobi left a comment •

edited

Loading