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

build: Add maven-compiler-plugin for java cross-build #911

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

viirya
Copy link
Member

@viirya viirya commented Sep 4, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Comment on lines +814 to +824
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.10.1</version>
<configuration>
<source>${java.version}</source>
<target>${java.version}</target>
<skipMain>true</skipMain>
<skip>true</skip>
</configuration>
</plugin>
Copy link
Member Author

@viirya viirya Sep 4, 2024

Choose a reason for hiding this comment

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

cc @huaxingao I guess that we need this to make a Comet build on Java 17 builder to be built with Iceberg on Java 11.

@viirya viirya changed the title build: Add maven-compiler-plugin build: Add maven-compiler-plugin for java cross-build Sep 4, 2024
@viirya
Copy link
Member Author

viirya commented Sep 5, 2024

After @huaxingao tested it, this doesn't help on the issue. I will close this for now.

@viirya viirya closed this Sep 5, 2024
@viirya viirya reopened this Sep 6, 2024
@viirya
Copy link
Member Author

viirya commented Sep 6, 2024

Wait for re-verifying

cc @huaxingao

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.01%. Comparing base (033fe6f) to head (0784bab).
Report is 10 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #911      +/-   ##
============================================
- Coverage     34.03%   34.01%   -0.03%     
+ Complexity      883      875       -8     
============================================
  Files           113      112       -1     
  Lines         43170    43221      +51     
  Branches       9516     9563      +47     
============================================
+ Hits          14693    14701       +8     
- Misses        25471    25491      +20     
- Partials       3006     3029      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@parthchandra
Copy link
Contributor

Seems to be the right thing to do irrespective of whether it addresses the iceberg build issue.

@viirya
Copy link
Member Author

viirya commented Sep 10, 2024

Thanks @parthchandra .

@viirya viirya merged commit f1becd1 into apache:main Sep 10, 2024
151 checks passed
@viirya viirya deleted the fix_build branch September 10, 2024 15:36
@viirya
Copy link
Member Author

viirya commented Sep 10, 2024

Thanks @andygrove @huaxingao @parthchandra

@parthchandra
Copy link
Contributor

Just realized that we already had

    <maven.compiler.source>${java.version}</maven.compiler.source>
    <maven.compiler.target>${java.version}</maven.compiler.target>

which set these properties for the compiler plugin. So it appears we are not adding anything new, which may explain why this did not help.

There is a new property available since JDK 9 which we could use (and also remove the jdk 1.8 profile which is meaningless if the source and target version are set to 11 or greater)

<maven.compiler.release>${java.version}</maven.compiler.release>

https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html

@huaxingao

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.

5 participants