Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try different jdk versions in sequence #583

Merged
merged 4 commits into from
May 11, 2023
Merged

Conversation

keynmol
Copy link
Contributor

@keynmol keynmol commented May 11, 2023

  • Preload JDK 17, 11, 8 into the container
  • Try each one until the indexing succeeds

The order might be wrong, may be should swing wildly - 17, then 8, then 11.

This change allows indexing https://github.com/indeedeng/proctor - first it tries 17 and 11, fails on both, then 8, produces a valid SCIP index.

Additionally:

  • Switch to a base JDK 17 image

  • Install Gradle the same way we install Maven, for consistency

    This also removes gradle from being the entrypoint of the docker image, which was confusing
    Good time to make scip-java itself the entry point

Test plan

  • Add problematic repo to docker CLI test list

@keynmol keynmol marked this pull request as ready for review May 11, 2023 09:47
@keynmol keynmol requested a review from olafurpg May 11, 2023 09:48
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. I've been wondering for a while if this is the way to go but didn't like the idea of running scip-java index in a for loop, and I can imagine some pathological case where the indexing step fails very late causing the indexing job to become 3x slower (and eventually Nx slower when we have N JVM versions)

@@ -45,10 +45,15 @@ jobs:
run: sbt cli/docker

- run: |
git clone https://github.com/circe/circe.git
set -eu
for REPO in circe/circe indeedeng/proctor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth making this a matrix build so these can fail independently? Also in parallel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make this parallel within the job itself, but given that I hope to have 5-10 repos in here, I think this will make matrix build too big - and each job will have to suffer the penalty of publishing docker CLI.

In a next PR I will make this faster by using GNU parallel and mounting cache folders into the containers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. It's very valuable to have a complete e2e testing pipeline but if the job becomes too slow then we can consider making it optional in PRs behind some GH label. Many PRs don't influence auto-indexing stuff and it sucks when the CI takes +30 minutes

@keynmol keynmol merged commit d16df28 into main May 11, 2023
@keynmol keynmol deleted the try-different-jdk-versions branch May 11, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants