Build only changed examples and do it parallel #559

ihostage · 2024-03-01T12:29:16Z

Main reason: decrease a build time

Target behaviour:

For Pull Requests check only changed examples
For long-live branches check all examples
Check any example in separated job

ihostage

@mkurz Can you check/approve my idea so that I'm going to continue this work?

ihostage · 2024-03-05T14:26:17Z

.github/workflows/build-test.yml

-      cmd: |
-        ./test.sh --netty
+  changes:
+    if: github.event_name == 'pull_request'


Detect changes only for PR builds

ihostage · 2024-03-05T14:26:59Z

.github/workflows/build-test.yml

+      - uses: dorny/paths-filter@v3
+        id: filter
+        with:
+          filters: |


create named filter per example

ihostage · 2024-03-05T14:28:14Z

.github/workflows/build-test.yml


  finish:
    name: Finish
-    if: github.event_name == 'pull_request'
+    if: ${{ github.event_name == 'pull_request' && !failure() && !cancelled() }}


Success if depends was skipped

mkurz · 2024-03-05T14:46:07Z

@mkurz Can you check/approve my idea so that I'm going to continue this work?

Looking now.

mkurz · 2024-03-05T15:07:02Z

.github/workflows/build-test.yml

-  gradle-tests:
-    name: Gradle Tests
-    uses: playframework/.github/.github/workflows/cmd.yml@v3
+  java-chatroom:


So does this mean you have to duplicate this section for each samples, like 31 times? Each time basically the same except a different sample name?

Yes, you are right. Potentially, we can try to pass needs.changes.outputs.examples array as another dimension for matrix job, but because I need to do that only once, currently I prefer duplicate jobs per example to have a full control of build logic for every example.

It's a reasonable cost to me to get the target. But of course it's discussable 😉

Thinking...

mkurz · 2024-03-05T15:08:05Z

.github/workflows/build-test.yml

+          find -name build.sbt | xargs sed -i 's/\/\/.enablePlugins(PlayNettyServer)/.enablePlugins(PlayNettyServer)/g'
+        fi
+        sbt ++$MATRIX_SCALA test
+        ./gradlew -Dscala.version="$MATRIX_SCALA" check


so this needs to be duplicated 31 times?

To me it isn't duplicate, it's a moving from scripts folder in every examples, that I want to delete in the future 😂 But technically, you're absolutely right.

But this introduced much more duplication, we have

find -name build.sbt | xargs sed -i 's/\/\/.enablePlugins(PlayNettyServer)/.enablePlugins(PlayNettyServer)/g'

just once in the test.sh file now, but now you have to add that (and the if condition) 31 times...

mkurz · 2024-03-07T10:17:33Z

Sorry for the delay Sergey...
The problem for me is if we move everything into the yml file and remove the script files (./test.sh and the scripts within the scripts/ folder of each sample), I will not be able to run the tests locally anymore (without maintaining my own script files). I actually do run the test scripts locally quite often... (I just need to export MATRIX_SCALA=... and then run ./test.sh or if testing a single sample just run that sample's script). This way I also found the bug in Scala 3.3.2 which now even has it's own blog post..
So, personally, I would prefer to keep the script files (they will be excluded in the zip files anyway).

So what about if we do following?
Allow the test.sh script to take an additional parameter --sample=play-java-chatroom-example which you can use to pass the example to run. If --sample=... is missing all samples will run, just like before.
So, together with chatgtp I came up with following modified test.sh script (so not 100% sure if it really works because "AI" 😉):

#!/usr/bin/env bash
if [ -z "$MATRIX_SCALA" ]; then
    echo "Error: the environment variable MATRIX_SCALA is not set"
    exit 1
fi

# Initialize variables
netty=false
sample=""

# Parse command line arguments
while [[ "$#" -gt 0 ]]; do
  case $1 in
    --netty) netty=true ;;  # Set netty to true if --netty is passed
    --sample=*) sample="${1#*=}" ;;  # Extract the value right of the equal sign
    *) echo "Unknown parameter passed: $1"; exit 1 ;;
  esac
  shift  # Move to next argument
done

if [ "$netty" = true ]; then
  find -name build.sbt | xargs sed -i 's/\/\/.enablePlugins(PlayNettyServer)/.enablePlugins(PlayNettyServer)/g'
fi

if [ -n "$sample" ]; then
  # Sample is set and not empty
  pushd $sample                           && scripts/test-sbt && popd
else
  # sample not set, therefore run all samples
  pushd play-java-chatroom-example        && scripts/test-sbt && popd
  pushd play-java-compile-di-example      && scripts/test-sbt && popd
  # ...
  # ... and all other samples just like we have now
fi

It should be possble now to call the script from the workflow yml with

./test.sh --netty --sample=play-java-chatroom-example if you duplicate the jobs 31 times in the yml files (which I do not like so much)
./test.sh --netty --sample=$MATRIX_SAMPLE if we add the output json array as another dimension for matrix job (which I actually prefer to avoid so much duplication in the yml file and also now I think with my proposed solution this makes much more sense because we keep the logic in the sh scripts anyway, so there is not much to control anymore in the yml file...)

And now even better:
I saw you introduced a $MATRIX_SERVER, so instead of having a --netty flag we should just introduce a --backend-server=netty flag in the test.sh script. If the flag is present and has the value netty then the netty server is used, otherwise just fall back to pekko as default (if no flag passed or if its value is unrecognized like asdf).
This way we do not even have to make that if condition just to pass the --netty flag and we can build a very nice matrix to run all the 62 jobs (31 samples x 2 backend servers) with just one line in a single job (actually even more jobs will run because the matrix is even bigger with java: 17, 11, scala: 2.13.x, 3.x):

./test.sh --backend-server=$MATRIX_SERVER --sample=$MATRIX_SAMPLE

What do you think about that?
My solution keeps the scripts so I/we can still run everything locally, but also allows us to parallelize all samples executions on CI in parallel. The only thing you don't have is full control in the yml file, but you still have them in the sh files (which is not bad IMHO, so if you look the the test script it's directly in the samples folder...)

mkurz · 2024-03-07T10:34:38Z

Aso one more idea (in addition to my last comment):

If you want we can remove some of the <sample>/scripts/test-sbt files, because some of them just are duplicates and just run sbt ++$MATRIX_SCALA test so in the bash script we could check if such a test-sbt file exists and only run it when it does exist and if not just fall back to sbt ++$MATRIX_SCALA test in the test.sh script itself.
So we keep only some test-sbt scripts which are doing specific things.
We don't have to do that, just an additional idea. WDYT?

ihostage · 2024-03-07T13:25:25Z

So, Matthias, I totally agree with many of your points 👍
And I remember that you use this common script to check samples locally 😉
Can you check a new version locally again?

ihostage · 2024-03-07T13:32:47Z

It should be possble now to call the script from the workflow yml with

./test.sh --netty --sample=play-java-chatroom-example if you duplicate the jobs 31 times in the yml files (which I do not like so much)

./test.sh --netty --sample=$MATRIX_SAMPLE if we add the output json array as another dimension for matrix job (which I actually prefer to avoid so much duplication in the yml file and also now I think with my proposed solution this makes much more sense because we keep the logic in the sh scripts anyway, so there is not much to control anymore in the yml file...)

One main reason to me of using duplicates in YAML is visualization in Action page. If we will use samples as one matrix dimension we will have a long ungrouped list of jobs and finding in this list one needed job log will be very complicated 😞

test.sh

.github/workflows/build-test.yml

mkurz · 2024-03-07T13:53:47Z

Can you check a new version locally again?

I checked locally and the scripts works for me 👍 Added some comments.

One main reason to me of using duplicates in YAML is visualization in Action page. If we will use samples as one matrix dimension we will have a long ungrouped list of jobs and finding in this list one needed job log will be very complicated 😞

For me it is ok if you add the 31 jobs manually and not adding it to the matrix if you think it's better. We will have much less duplication now anyway, it should always be just one line I think?

One more thing, which is just for you and you probably know anyway. Currently using netty together with gradle does not "work", will still use pekko. So you maybe want to make this work also, but it's up to you:

... --backend-server=netty --build-tool=gradle # this combination does not work yet

mkurz · 2024-03-07T14:41:26Z

One main reason to me of using duplicates in YAML is visualization in Action page. If we will use samples as one matrix dimension we will have a long ungrouped list of jobs and finding in this list one needed job log will be very complicated 😞

Actually, a bit of a stupid question, but why do you want to find a specific job within the matrix?
I think only when a job fails you click on it and then you see anyway which job it was (in the title or in the logs).
Not sure if we really are looking for a specific job very often....

ihostage · 2024-03-07T18:43:12Z

Not sure if we really are looking for a specific job very often....

Yes, but sometimes it can be useful and our workflow should be ready to this case 😄

Actually, a bit of a stupid question, but why do you want to find a specific job within the matrix?

Maybe in the future we can back to this discussion, but currently I see the next cases when separated jobs are useful:

gradle-build-root property. I don't remember, but it can be trouble using matrix variable as value of this property on the same level as matrix definition
I change a few samples and want to be sure that all changes was checked = check the name of samples on the second column in action summary page or list of jobs on the left side of page
Temporary add/update/remove matrix settings for one sample. I mean Java/Scala/Sbt/Gradle/Pekko/Netty settings for concrete sample. Juggling (include/exclude) settings of unified matrix isn't as I want to do and it isn't a quickly 😄
Simplification sequential working on New folder structure #558

.github/workflows/build-test.yml

mkurz · 2024-03-08T12:23:08Z

play-java-grpc-example/scripts/test-sbt

-echo "+----------------------------+"
-echo "| Executing tests using sbt  |" 
-echo "+----------------------------+"
-sbt ++$MATRIX_SCALA test docs/paradox


You removed docs/paradox? Why?

Yep, removed. This docs had been publishing in Lightbend Developer Portal, but we don't publish it anymore. That why I think it isn't actual for us now and we don't need to check it during build. We should migrate this docs to Antora in the future 🤔

mkurz · 2024-03-08T12:23:32Z

play-scala-grpc-example/scripts/test-sbt

-echo "+----------------------------+"
-echo "| Executing tests using sbt  |" 
-echo "+----------------------------+"
-sbt ++$MATRIX_SCALA test docs/paradox


Again docs/paradox removed?

mkurz · 2024-03-11T11:11:02Z

Hmm... why was

and

skipped?

Maybe because the java file upload sample failed and these two started later so they did not even start running because of !failure()?

Should we maybe just restart all jobs once more to see if they pass? Maybe can you rebase?

ihostage · 2024-03-11T11:16:04Z

Hmm... why was

and

skipped?

Because these samples didn't change 😉

mkurz

Because these samples didn't change 😉

Yeah, that makes sense 😉

If you are finished you can merge this. I am not totally happy about that many jobs and its duplicated lines in the yml file but I can life with it. I have a bit of a feeling we might overengineer things here... But let's give it a try.

ihostage · 2024-03-11T11:49:37Z

@Mergifyio backport 3.0.x

mergify · 2024-03-11T11:49:42Z

backport 3.0.x

✅ Backports have been created

#581 [3.0.x] Build only changed examples and do it parallel (backport #559) by @ihostage has been created for branch 3.0.x but encountered conflicts

ihostage · 2024-03-11T11:49:58Z

@Mergifyio backport 2.9.x

mergify · 2024-03-11T11:50:04Z

backport 2.9.x

✅ Backports have been created

#582 [2.9.x] Build only changed examples and do it parallel (backport #559) by @ihostage has been created for branch 2.9.x but encountered conflicts

ihostage force-pushed the parallel-build branch 19 times, most recently from 2f97bf4 to 3cb0e06 Compare March 5, 2024 13:57

ihostage changed the title ~~Build examples in parallel~~ Build only changed examples and do it parallel Mar 5, 2024

ihostage commented Mar 5, 2024

View reviewed changes

ihostage requested a review from mkurz March 5, 2024 14:29

mkurz reviewed Mar 5, 2024

View reviewed changes

ihostage force-pushed the parallel-build branch 3 times, most recently from e3e4e31 to 698bd4d Compare March 7, 2024 12:16

ihostage force-pushed the parallel-build branch from a2ff598 to 8133278 Compare March 7, 2024 13:06

mkurz reviewed Mar 7, 2024

View reviewed changes

test.sh Outdated Show resolved Hide resolved

mkurz reviewed Mar 7, 2024

View reviewed changes

test.sh Outdated Show resolved Hide resolved

mkurz reviewed Mar 7, 2024

View reviewed changes

test.sh Show resolved Hide resolved

mkurz reviewed Mar 7, 2024

View reviewed changes

.github/workflows/build-test.yml Outdated Show resolved Hide resolved

ihostage force-pushed the parallel-build branch 3 times, most recently from 664d4fa to ce4a611 Compare March 7, 2024 14:26

ihostage force-pushed the parallel-build branch 2 times, most recently from 524ced6 to b08484a Compare March 7, 2024 18:17

ihostage marked this pull request as ready for review March 7, 2024 18:46

mkurz requested changes Mar 8, 2024

View reviewed changes

Build only changed examples and do it parallel

3549d9b

ihostage force-pushed the parallel-build branch from b08484a to 3549d9b Compare March 8, 2024 18:54

mkurz approved these changes Mar 11, 2024

View reviewed changes

ihostage merged commit 051d2e1 into playframework:main Mar 11, 2024
313 checks passed

ihostage deleted the parallel-build branch March 11, 2024 11:48

mergify bot mentioned this pull request Mar 11, 2024

[3.0.x] Build only changed examples and do it parallel (backport #559) by @ihostage #581

Merged

mergify bot mentioned this pull request Mar 11, 2024

[2.9.x] Build only changed examples and do it parallel (backport #559) by @ihostage #582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build only changed examples and do it parallel #559

Build only changed examples and do it parallel #559

ihostage commented Mar 1, 2024 •

edited

ihostage left a comment

ihostage Mar 5, 2024

ihostage Mar 5, 2024

ihostage Mar 5, 2024

mkurz commented Mar 5, 2024

mkurz Mar 5, 2024

ihostage Mar 5, 2024

ihostage Mar 5, 2024

mkurz Mar 5, 2024

mkurz Mar 5, 2024

ihostage Mar 5, 2024

mkurz Mar 7, 2024

mkurz commented Mar 7, 2024

mkurz commented Mar 7, 2024 •

edited

ihostage commented Mar 7, 2024

ihostage commented Mar 7, 2024

mkurz commented Mar 7, 2024

mkurz commented Mar 7, 2024 •

edited

ihostage commented Mar 7, 2024 •

edited

mkurz Mar 8, 2024

ihostage Mar 8, 2024

mkurz Mar 8, 2024

mkurz commented Mar 11, 2024

ihostage commented Mar 11, 2024

mkurz left a comment

ihostage commented Mar 11, 2024

mergify bot commented Mar 11, 2024 •

edited

ihostage commented Mar 11, 2024

mergify bot commented Mar 11, 2024 •

edited

Build only changed examples and do it parallel #559

Build only changed examples and do it parallel #559

Conversation

ihostage commented Mar 1, 2024 • edited

ihostage 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

mkurz commented Mar 5, 2024

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

mkurz commented Mar 7, 2024

mkurz commented Mar 7, 2024 • edited

ihostage commented Mar 7, 2024

ihostage commented Mar 7, 2024

mkurz commented Mar 7, 2024

mkurz commented Mar 7, 2024 • edited

ihostage commented Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz commented Mar 11, 2024

ihostage commented Mar 11, 2024

mkurz left a comment

Choose a reason for hiding this comment

ihostage commented Mar 11, 2024

mergify bot commented Mar 11, 2024 • edited

✅ Backports have been created

ihostage commented Mar 11, 2024

mergify bot commented Mar 11, 2024 • edited

✅ Backports have been created

ihostage commented Mar 1, 2024 •

edited

mkurz commented Mar 7, 2024 •

edited

mkurz commented Mar 7, 2024 •

edited

ihostage commented Mar 7, 2024 •

edited

mergify bot commented Mar 11, 2024 •

edited

mergify bot commented Mar 11, 2024 •

edited