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

Add AWS-LC src & ref flags to Gradle build #390

Merged
merged 8 commits into from
Jul 2, 2024

Conversation

sp717
Copy link
Contributor

@sp717 sp717 commented Jul 1, 2024

Issue #, if available:

Description of changes:
In case a user needs to build ACCP with a specific version of AWS-LC, this change adds 2 flags that can be passed to Gradle to specify a source directory for AWS-LC or a specific Git reference.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sp717 sp717 marked this pull request as ready for review July 1, 2024 19:01
@sp717 sp717 requested a review from a team as a code owner July 1, 2024 19:01
build.gradle Outdated Show resolved Hide resolved
@geedo0
Copy link
Contributor

geedo0 commented Jul 1, 2024

What's the impetus for this change? Improving developer experience or is this feeding into new CI scripts?

build.gradle Outdated

// Check for user inputted git version ID.
if (System.properties["AWSLC_REF"]) {
ext.awsLcGitVersionId = System.properties["AWSLC_REF"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm pretty sure you meant to use System.env.AWSLC_REF. What you have here fetches from the JVM System Properties which is a totally different thing.

https://docs.gradle.org/current/userguide/build_environment.html#sec:gradle_environment_variables

Copy link
Contributor Author

@sp717 sp717 Jul 1, 2024

Choose a reason for hiding this comment

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

Would that still pull input from the gradle build command using -D (example below)? I tested the change and it didn't seem to work the same way, but I may have misunderstood.

ex:
./gradlew build -DAWSLC_REF="version"

Copy link
Contributor

Choose a reason for hiding this comment

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

I reviewed the conventions in our Gradle script, it looks like we've developed a preferences for using Java system properties like this. What you have here is consistent and that's good. Bigger picture, the nice thing about using environmental variables to manipulate a build is that they are managed external to Gradle and are part of the POSIX standard.

# Set them within the scope of your shell
export AWSLC_REF="version"
./gradlew build

# Set them within the scope of your gradle process
AWSLC_REF="version" ./gradlew build

# Set them as part of a python script executing a process...
env = {'AWSLC_REF' : 'version'}
subprocess.run(['gradlew', 'build'], env=env)

Copy link
Contributor

Choose a reason for hiding this comment

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

In retrospect, we should have used Gradles properties instead of JVM properties or even environment variables.

I'm not suggesting changing that in this PR since the use of JVM properties has been preferred.

@sp717
Copy link
Contributor Author

sp717 commented Jul 1, 2024

What's the impetus for this change? Improving developer experience or is this feeding into new CI scripts?

This change helps with updating AWS-LC's CI to include ACCP. The current ACCP build defaults to using v1.30.1 of AWS-LC, but we'd like to test against any new version of AWS-LC, which this update should solve.

geedo0
geedo0 previously approved these changes Jul 2, 2024
build.gradle Show resolved Hide resolved
@geedo0 geedo0 merged commit f505960 into corretto:main Jul 2, 2024
10 checks passed
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.

4 participants