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

Replace custom air.main.basedir property with built-in session.rootDirectory #20486

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

nineinchnick
Copy link
Member

Description

session.rootDirectory was introduced in Maven 3.9

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Jan 28, 2024
@github-actions github-actions bot added docs jdbc Relates to Trino JDBC driver tests:hive hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector mongodb MongoDB connector labels Jan 28, 2024
@nineinchnick nineinchnick mentioned this pull request Jan 28, 2024
@nineinchnick
Copy link
Member Author

It doesn't work with the Modernizer plugin and I'm not sure why.

pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@cstamas cstamas left a comment

Choose a reason for hiding this comment

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

This is a good PR, but modernizer needs a circumvention as described above.

@cstamas
Copy link
Member

cstamas commented Jan 28, 2024

This is a good PR, but modernizer needs a circumvention as described above.

@cstamas cstamas reopened this Jan 28, 2024
@wendigo
Copy link
Contributor

wendigo commented Jan 28, 2024

@nineinchnick nice

@cstamas
Copy link
Member

cstamas commented Jan 28, 2024

com.github.ekryd.sortpom:sortpom-maven-plugin:3.3.0:verify (default) on project trino-testing-services: The file /home/runner/work/trino/trino/testing/trino-testing-services/pom.xml is not sorted

So edits did violate some rules, needs reformat?

@nineinchnick
Copy link
Member Author

com.github.ekryd.sortpom:sortpom-maven-plugin:3.3.0:verify (default) on project trino-testing-services: The file /home/runner/work/trino/trino/testing/trino-testing-services/pom.xml is not sorted

So edits did violate some rules, needs reformat?

This tripped on empty <properties></properties> tags, I removed them.

@cstamas
Copy link
Member

cstamas commented Jan 28, 2024

More sortpom issues:
com.github.ekryd.sortpom:sortpom-maven-plugin:3.3.0:verify (default) on project trino-phoenix5-patched: The file /home/runner/work/trino/trino/lib/trino-phoenix5-patched/pom.xml is not sorted

@nineinchnick nineinchnick requested a review from electrum January 29, 2024 08:37
@@ -12,7 +12,6 @@
<artifactId>trino-jdbc</artifactId>

<properties>
<air.main.basedir>${project.parent.basedir}</air.main.basedir>
Copy link
Member

Choose a reason for hiding this comment

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

Per airbase docs

For a multi-module project, all other sub-modules must have this explicitly set to the root directory and therefore the following code

<properties>
  <air.main.basedir>${project.parent.basedir}</air.main.basedir>
</properties>

must be added to each pom.

https://github.com/airlift/airbase/blob/master/README.md#airmainbasedir

so when using airbase root pom, we are expected to define the air.main.basedir property correctly, and this should not be removed

howeever, airbase seems to make no direct use of air.main.basedir property, so we may perhaps simply remove it from airbase (remove from airbase pom and readme). this would be a prep step, if we're pedantic.

Copy link
Member Author

Choose a reason for hiding this comment

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

this would be a prep step, if we're pedantic.

Oh, but we are!

Airbase 149 updates the plugin to 7.0.0, and this version fixes the issue.
@wendigo
Copy link
Contributor

wendigo commented Feb 15, 2024

I like the fact that we are making POMs smaller :)

@wendigo
Copy link
Contributor

wendigo commented Feb 15, 2024

@nineinchnick do you plan a follow-up in Airbase to remove this property?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed delta-lake Delta Lake connector docs hive Hive connector hudi Hudi connector iceberg Iceberg connector jdbc Relates to Trino JDBC driver mongodb MongoDB connector
Development

Successfully merging this pull request may close these issues.

4 participants