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 maven local by default in order to test local build (especially for Apache Iceberg) #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbonofre
Copy link
Member

@jbonofre jbonofre commented Sep 4, 2024

No description provided.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

mavenLocal() comes with a bunch of drawbacks. In particular it slows down the build and it prevents Gradle from verifying that dependencies come from the "right" source (repository validations don't work).

I'd propose two changes here:

  1. Make the addition of mavenLocal() dependent on a system or project property (example here). No need to slow down the build for everyone.
  2. Use Gradle included builds - include the Iceberg build into the Polaris build. Then you also get all the benefits of Gradle and can skip the publishing step entirely. It also allows debugging into the Iceberg code.

@jbonofre
Copy link
Member Author

jbonofre commented Sep 4, 2024

@snazy thanks ! I will update the PR according to your comments.

@jbonofre
Copy link
Member Author

jbonofre commented Sep 4, 2024

@snazy just to be sure, by "included builds", you mean using --include-build arg right ?

@snazy
Copy link
Member

snazy commented Sep 4, 2024

@snazy just to be sure, by "included builds", you mean using --include-build arg right ?

Either this - or in the build scripts - the result's the same

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.

2 participants