Skip to content

Conversation

@stoty
Copy link
Contributor

@stoty stoty commented Nov 12, 2024

…build environment

@stoty stoty requested a review from F21 November 12, 2024 09:53
@stoty stoty force-pushed the CALCITE-6687 branch 2 times, most recently from 8eae6f0 to ecb6c53 Compare November 12, 2024 12:00
Copy link
Member

@zabetak zabetak left a comment

Choose a reason for hiding this comment

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

How we can be sure that constraints are enforced correctly? In other words, how do we test this PR?

Copy link
Member

@zabetak zabetak left a comment

Choose a reason for hiding this comment

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

LGTM, the only thing that I would change before merging is to read the property from the gradle.properties file as suggested.

On a more general topic, we should not be forced to fine-tune deps for plugins but rather upgrade the version plugin itself. I understand that we cannot do it now since JDK8 is unsupported in more recent version but I guess we should start discussing till when we are going to support the JDK8 on our side. Eventually we are gonna reach a point that workarounds like this one are not gonna work.

@stoty
Copy link
Contributor Author

stoty commented Jan 14, 2025

How we can be sure that constraints are enforced correctly? In other words, how do we test this PR?

#257 fails with the default asm version. If that builds successfully, then the constraints are enforced.

@zabetak
Copy link
Member

zabetak commented Jan 14, 2025

We can also check that constraints are enforced via ./gradlew buildEnvironment or by publishing a build scan.

@stoty
Copy link
Contributor Author

stoty commented Jan 14, 2025

Thank you, I have added the suggested property references, this is a much better solution.

As for JDK 8 support, I agree that it is generally a pain, and is only getting worse, but some projects using Avatica still support JDK8.

@stoty
Copy link
Contributor Author

stoty commented Jan 14, 2025

rebased and merged commits

@stoty stoty merged commit 4800493 into apache:main Jan 14, 2025
12 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.

3 participants