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

feat: hermetic build scripts to use a single output/generation folder #1987

Merged
merged 27 commits into from Sep 14, 2023

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Sep 13, 2023

This enables the hermetic build scripts to generate and download everything into a single output folder, to ease the cleanup in case it fails

Showcase scripts were adjusted accordingly
README was adjusted to explain the new structure

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 13, 2023
@diegomarquezp diegomarquezp changed the title feat: hermetic build script to use a single output/generation folder feat: hermetic build scripts to use a single output/generation folder Sep 13, 2023
@diegomarquezp diegomarquezp marked this pull request as ready for review September 13, 2023 16:15
@diegomarquezp diegomarquezp requested a review from a team as a code owner September 13, 2023 16:15
@diegomarquezp diegomarquezp marked this pull request as draft September 13, 2023 16:16
@diegomarquezp diegomarquezp marked this pull request as ready for review September 13, 2023 17:39
library_generation/README.md Outdated Show resolved Hide resolved
library_generation/README.md Outdated Show resolved Hide resolved
library_generation/generate_library.sh Outdated Show resolved Hide resolved
library_generation/utilities.sh Outdated Show resolved Hide resolved
# compare with gapic library
(diff -ru "${SHOWCASE_DIR}/${GAPIC_PROJECT_DIR}/src/main/java" "${SCRIPT_DIR}/output/showcase-output/gapic-showcase-output/src/main/java")
} || {
failure="true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this logic correct? I see that showcase integration tests always exit with 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is correct, I found out why they were failing. It was because there was a pushd cd (fixed in 0b54240)

Now mvn verify -P enable-golden-tests behaves correctly

Co-authored-by: Joe Wang <106995533+JoeWang1127@users.noreply.github.com>
library_generation/README.md Outdated Show resolved Hide resolved
library_generation/README.md Outdated Show resolved Hide resolved
library_generation/generate_library.sh Outdated Show resolved Hide resolved
@@ -45,11 +45,14 @@ done
script_dir=$(dirname "$(readlink -f "$0")")
source "${script_dir}/../utilities.sh"
library_generation_dir="${script_dir}"/..
cd "${library_generation_dir}"
output_folder="$(pwd)/output"
mkdir -p "${output_folder}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think output_folder will be created by generate_library.sh, does it need to be created 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.

we need external repositories to be located in there as well, which is handled by generate_showcase.sh and generate_library_integration_test.sh. I guess it would make more sense to rely on a function to prevent duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0ebc098

@@ -181,15 +181,14 @@ generate_library_failed_with_invalid_generator_version() {
local destination="google-cloud-alloydb-v1-java"
local res=0
cd "${script_dir}/resources"
$("${script_dir}"/../generate_library.sh \
"${script_dir}"/../generate_library.sh \
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason I wrap this command into $() is because generate_library.sh will exit if the version is incorrect and use $() to force back to the current shell and execute command after \\.

I'm not sure whether this is the best approach though, maybe worth an investigation later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this approach is good either but I prepended bash $script in 7fc632a to prevent the whole test suite from exiting if that's what you mean by force back to the current shell. I was having issues when running the unit tests after the adding the OS detection (or a few commits later) function which happened to be fixed by removing $().

Copy link
Collaborator

@JoeWang1127 JoeWang1127 left a comment

Choose a reason for hiding this comment

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

LGTM with a minor comment but wait for @blakeli0 comment.

@@ -1,6 +1,8 @@
#!/usr/bin/env bash

set -xeo pipefail
utilities_script_dir=$(echo "${BASH_SOURCE}" | rev | cut -d/ -f2- | rev)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment to explain what this line does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoeWang1127 I forgot we are calling this script from wherever, leaving our output folder at mercy of the user's CWD instead of forcing it to be in library_generation. I reverted this change but still left the utility function now relying on $(pwd)

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 2023

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 2023

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 2023

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@diegomarquezp diegomarquezp merged commit f5efb0e into main Sep 14, 2023
41 of 43 checks passed
@diegomarquezp diegomarquezp deleted the feat/hermetic-build/single-folder-output branch September 14, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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