-
Notifications
You must be signed in to change notification settings - Fork 363
Add client build to Gradle #2590
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
Conversation
HonahX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick PR! Seems it need rebase to pick up the fix for cryptography license issue
| description = "Build the python client" | ||
|
|
||
| workingDir = project.projectDir | ||
| commandLine("make", "client-build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts: Shall we add an option to build only sdist? Underlying it would be
poetry build --format=sdist
Could be useful in some case when we only want sdist : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts: Shall we add an option to build only sdist? Underlying it would be
poetry build --format=sdistCould be useful in some case when we only want sdist : )
Good suggestion. Let me quickly add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HonahX this is added. Instead of hard-coded it, it is now controlled via FORMAT when using the Makefile such as following:
# build both
make client-build
# build sdist
FORMAT=sdist make client-build
# build wheel
FORMAT=wheel make client-build
When using gradle, it is control via python.format:
# build both
./gradlew buildPythonClient
# build sdist
./gradlew buildPythonClient -Ppython.format=sdist
# build wheel
./gradlew buildPythonClient -Ppython.format=wheel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks great! Thanks for adding it!
| excludes.add("**/*.png") | ||
| } | ||
|
|
||
| tasks.register<Exec>("buildPythonClient") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it hooked up to any "regular" top-level commands?
Should we run it in CI for verification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like I merged too early. Currently it is not linked with any top level commands, but that is possible if we want to do so. The current CI is using the make command only. There is a WIP PR which needs this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking to top-level commands is not critical... I was just wondering. If we connect it, I'd suggest connecting to the assemble chain.
Validating this new tasks in CI would be really good to have, but it's ok to do that in a follow-up PR, of course :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. Let me review this later tonight or tomorrow then purpose a new PR.
* docs(README): Create Polaris-Core README (apache#2585) * docs(README): Create Polaris-Core README * Add Events for Iceberg REST APIs (apache#2480) * Update dependency com.google.guava:guava to v33.5.0-jre (apache#2601) * Bump: Iceberg client in tests and documentation to 1.10 (apache#2588) * Add client build to Gradle (apache#2590) * Add client build to Gradle * Add overwrite option for python build * Match client build behavior * Add content to contributing guidelines. (apache#2536) Add community guidelines. * CI/Caching: Fix Gradle cache retention (apache#2604) Older versions of Gradle's setup-gradle action did not actively trigger stale cache entry cleanup, which is why there was a separate step "Trigger Gradle home cleanup" in `gradle.yml`. Nowadays, that action triggers a "noop build" to explicitly trigger stale cache entry cleanup, but uses somewhat different defaults than [described here](https://docs.gradle.org/current/userguide/directory_layout.html#dir:gradle_user_home:configure_cache_cleanup). This change adds an explicit configuration for Gradle cache cleanup/retention with reasonable values considering the total 10GB limit for all GitHub caches per repository. The change described above lead to a behavioral change, which evicts all non-accessed cache entries within the current GH workflow job, which effectively evicted all cache entries from dependent jobs - in other words: the (build) cache was nearly empty, leading to full rebuilds. This behavior could be observed in the output of the "Post Collect partial Gradle build caches"-step in the log message "Build cache (/home/runner/.gradle/caches/build-cache-1) removing files not accessed on or after ..." showing the timestamp when the setup-gradle action was started. * Last merged commit d8602f6 --------- Co-authored-by: Adam Christian <105929021+adam-christian-software@users.noreply.github.com> Co-authored-by: Adnan Hemani <adnan.h@berkeley.edu> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com> Co-authored-by: Yong Zheng <yongzheng0809@gmail.com> Co-authored-by: JB Onofré <jbonofre@apache.org>
This is for adding a client build task to Gradle as requested via https://github.com/apache/polaris/pull/2530/files:
As @HonahX pointed out, poetry only build wheel specific for the OS/hardware where this was executed. Thus, if we want to include diff wheel files (one for each support OS/hardware), we will need to do it via cibuildwheel instead. However, if we only need the dist of code,
client/python/dist/polaris-1.1.0.tar.gzis safe to use and it is installable as well: