Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.

ci: downstream check for native image tests #771

Merged
merged 4 commits into from
Aug 3, 2022
Merged

Conversation

mpeddada1
Copy link
Contributor

For reference: googleapis/gax-java#1681

@mpeddada1 mpeddada1 requested a review from a team as a code owner August 2, 2022 14:41
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Aug 2, 2022
@@ -59,7 +62,5 @@ if [[ $CLIENT_LIBRARY == "bigtable" ]]; then
popd
fi

mvn verify install -B -V -ntp -fae \
Copy link
Member

Choose a reason for hiding this comment

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

"test" type in build.sh seems to have the following command:

retry_with_backoff 3 10 \
  mvn install -B -V -ntp \
    -DskipTests=true \
    -Dclirr.skip=true \
    -Denforcer.skip=true \
    -Dmaven.javadoc.skip=true \
    -Dgcloud.download.skip=true \
    -T 1C

mvn test -B -ntp -Dclirr.skip=true -Denforcer.skip=true

which I'm not sure is equivalent to what you're replacing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks for catching this. I was trying to emulate the changes we made in gax-java but just realized that this line was recently reverted back to its original form in googleapis/gax-java@c5eb06e. Sorry about the confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An update: it looks like updating that line to its original form is causing the native image compilation to be skipped for graal tests.

@@ -156,4 +156,4 @@ jobs:
- run: java -version
- run: sudo apt-get update -y
- run: sudo apt-get install libxml2-utils
- run: .kokoro/downstream-client-library-check.sh ${{matrix.repo}}
- run: .kokoro/downstream-client-library-check.sh ${{matrix.repo}} test
Copy link
Member

Choose a reason for hiding this comment

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

Why are you modifying the non-GraalVM downstream check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The downstream checks will accept a JOB_TYPE as a second argument to distinguish between a standard run and graalvm run.

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Aug 2, 2022
@mpeddada1
Copy link
Contributor Author

@meltsufin there may be a couple more changes needed. I realized that the graal checks are passing but skipping the native compilation step which is usually represented by the following output:

Executing: /opt/hostedtoolcache/GraalVM/java11-linux-amd64-21.3.2/x64/bin/native-image @/tmp/native-image4009276204974001490args org.graalvm.junit.platform.NativeImageJUnitLauncher
Warning: Ignoring server-mode native-image argument --no-server.
[native-tests:4109]    classlist:  11,413.77 ms,  1.69 GB
[native-tests:4109]        (cap):     874.27 ms,  1.69 GB

@@ -18,6 +18,9 @@ set -eo pipefail
set -x

CLIENT_LIBRARY=$1

# Example JOB_TYPE: "test" or "graalvm"
export JOB_TYPE=$2
Copy link
Member

Choose a reason for hiding this comment

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

This variable is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as you noticed, the JOB_TYPE env variable is not getting picked up. The initial line ./kokoro/build.sh was able to run the tests in either standard Java or with native image compilation based on the env variable (as seen in the jobs run in the first commit )but modifying it to be in original state still forces it to run with standard java. It looks like this same change needs to be made in gax as the most recent runs of the downstream checks have been skipping the native image compilation stage. For example: https://github.com/googleapis/gax-java/runs/7619427005?check_suite_focus=true

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused because the Github workflows don't seem to be calling build.sh at all. They're using downstream-client-library-check.sh, which doesn't have GraalVM logic in it.

Copy link
Contributor Author

@mpeddada1 mpeddada1 Aug 2, 2022

Choose a reason for hiding this comment

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

Ah sorry my previous statement was kinda confusing. We need two types of downstream workflows (represented by downstream-native-image and downstream.yaml). While the call to downstream-client-library-check.sh in downstream.yaml sets the JOB_TYPE env variable to "test", the call to downstream-client-library-check.sh in downstream-native-image.yaml will set the JOB_TYPE to "graalvm". downstream-client-library-check.sh will then use those values to do export JOB_TYPE=${arg_value}. We will need to change the script to call build.sh which runs the respective mvn commands based on the JOB_TYPE present.

That being said, this change cause an extra command to run for standard java as you pointed out:mvn test -B -ntp -Dclirr.skip=true -Denforcer.skip=true

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Aug 2, 2022
@mpeddada1
Copy link
Contributor Author

Downstream native image checks are running as expected:

Screen Shot 2022-08-02 at 4 41 57 PM

@blakeli0 blakeli0 added the automerge Merge the pull request once unit tests and other checks pass. label Aug 2, 2022
@gcf-merge-on-green
Copy link

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 3, 2022
@meltsufin meltsufin merged commit 92d217c into main Aug 3, 2022
@meltsufin meltsufin deleted the native-image-downstream branch August 3, 2022 15:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants