Skip to content

Add maven profile for -Xlint:deprecation compiler option#12628

Closed
leveyja wants to merge 1 commit intotrinodb:masterfrom
leveyja:leveyja/enable-deprecation-warnings
Closed

Add maven profile for -Xlint:deprecation compiler option#12628
leveyja wants to merge 1 commit intotrinodb:masterfrom
leveyja:leveyja/enable-deprecation-warnings

Conversation

@leveyja
Copy link
Copy Markdown
Member

@leveyja leveyja commented Jun 1, 2022

Description

Adding maven profile to add -Xlint:deprecation compiler option.
Compiling with ./mvnw [...] -Pdeprecation will warn about deprecated method and class usages.

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Build compiler change to enable verbose deprecation warnings during compilation.

How would you describe this change to a non-technical end user or system administrator?

Adding a build option to enabling warnings to be printed during the build.

Related issues, pull requests, and links

Documentation

(:white_check_mark:) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(:white_check_mark:) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Copy link
Copy Markdown
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

Thanks!

@leveyja leveyja force-pushed the leveyja/enable-deprecation-warnings branch 3 times, most recently from 914f05b to 7e65dde Compare June 3, 2022 06:50
@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 3, 2022

Failed due to TestIcebergSparkCompatibility > testTrinoReadsSparkRowLevelDeletesWithRowTypes(0: ORC, 1: AVRO) [groups: profile_specific_tests, iceberg]
Will re-run

Compiling with ./mvnw [...] -Pdeprecation will warn about deprecated method and class usages.
@leveyja leveyja force-pushed the leveyja/enable-deprecation-warnings branch from 7e65dde to ff78bf1 Compare June 3, 2022 14:52
@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 6, 2022

Looks ready to merge - any suggestions who should give the final 👍 ?

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Unused profiles aren't useful - since people won't generally run with this profile.

Maybe you can add a new job to ci.yml that prints out a deprecation log with using this profile for running maven-checks.

But I think #12629 is the way to go instead - i.e. fix/disable current deprecations and then enable -Xlint:deprecation -Werror in the maven-checks job.

@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 20, 2022

Unused profiles aren't useful

It could be added to the errorprone-compiler profile instead? I was trying to keep it clean/simple by adding a separate profile (and anyone can read the pom.xml and understand what the profile does, so it's there to be used).
(Also, nit: I would use it - so it wouldn't be unused ;-) )

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Jan 15, 2024

Is this still valid or did we implement a better approach already @hashhar @leveyja @wendigo

@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 5, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Sep 5, 2024
@kokosing kokosing closed this Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants