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 iceberg-build.properties to Jars and release process #5236

Merged
merged 3 commits into from
Jul 10, 2022

Conversation

rdblue
Copy link
Contributor

@rdblue rdblue commented Jul 10, 2022

This ensures that iceberg-build.properties is added to Jars produced by the Gradle build, even when building from a source tarball with no .git directory.

This builds on the generateGitProperties task added in #5228 and updates the build:

  • A new task, buildInfo ensures that build/iceberg-build.properties exists, either by copying it from the source root or by calling generateGitProperties (change in tasks.gradle)
  • All Jars will now include iceberg-build.properties from the root build folder (change in deploy.gradle)
  • The source release script will generate iceberg-build.properties and add it to the root folder for the release candidate tarball (changes in source-release.sh)

Note that this also changes how release tarballs are created. Previously, the source-release.sh script would create version.txt and commit the file, then tag the commit with version.txt. This wasn't possible for adding iceberg-build.properties because iceberg-build.properties needs to describe the git commit in the source tarball (and can't be included in the commit itself). Instead of adding the file to git, this adds both version.txt and iceberg-build.properties directly to the archive using git archive ... --add-file. A side benefit is that we will no longer have release tags off of the master branch.


# remove the uncommitted build files so they don't affect the current working copy
rm $projectdir/version.txt
rm $projectdir/iceberg-build.properties
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't needed before because the version.txt file was committed. When switching back to the master branch, it would be removed because it isn't present in the master branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be any commit specifically for the release or will it simply be the last commit that goes into the release branch? Previously the version.txt commit wound up being the source release commit.

To be clear, I am definitely in agreement on removing the git commit from this script. It has caused several people issues while testing releasing in the past or when the release script needed to be rerun for the same RC candidate.

@@ -51,7 +51,7 @@ try {
gitPropertiesName = 'iceberg-build.properties'
gitPropertiesResourceDir = file("${rootDir}/build")
extProperty = 'gitProps'
failOnNoGitDirectory = false
failOnNoGitDirectory = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this cause build verification to fail? If we're downloading the tarball and building from that will the build fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this actually doesn't do much. The plugin is configured in a try/catch after the Palantir version plugin. So if there is not .git directory, this code doesn't even run.

Eventually, we should see if this can be moved outside of the try/catch, which was my intent here. I think that if generateGitProperties is called and it can't, then we want to have a failure. That's why this introduces a new task, buildInfo that will use the properties file in the build root if it is present. The failure we want is when you have no .git directory and have no iceberg-build.properties file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried without the try catch and having gitProperties in the build file causes even simple tasks like gradle tasks to fail because the surrounding gitProperties block is not registered and has no meaning to gradle. Even if it’s not used, such as gradle clean.

So my observation was that the try-catch block will always be needed for any gitProperties block.

Copy link
Contributor

@kbendick kbendick Jul 10, 2022

Choose a reason for hiding this comment

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

I think this flag is more for the case that somebody overrides the .git directory - which the plug-in allows - and that overridden directory isn’t present (at least from my testing). But git is still present so the plug-in is able to be applied in general.

Ryan’s right that it doesn’t do much and the flag won’t cause the build to fail when using the tarball.

@rdblue
Copy link
Contributor Author

rdblue commented Jul 10, 2022

I created 0.14.0-rc0 to test out the source-release.sh script: https://dist.apache.org/repos/dist/dev/iceberg/apache-iceberg-0.14.0-rc0/

I also verified that release and everything looks good:

  • The iceberg-build.properties and version.txt files are present
  • All Jars that are built include the iceberg-build.properties file
  • The tag and hash in the properties file matches the release candidate

Copy link
Contributor

@danielcweeks danielcweeks left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

This LGTM.

@kbendick
Copy link
Contributor

kbendick commented Jul 10, 2022

Will this place iceberg-build.properties on the runtime classpath for subprojects? Including if used from the IDE or from testing?

Things in core/build/resources/** will be accessible from core’s runtime classpath when I tested. But I’m less sure about the top-level build/** files.

Possibly that’s going to be handled in a follow up though (if needed at all). I’m less familiar with gradle than you are and this differs slightly from the way I was testing it before, so this could be fine.

@rdblue rdblue merged commit 791e7e6 into apache:master Jul 10, 2022
@rdblue
Copy link
Contributor Author

rdblue commented Jul 10, 2022

@kbendick, #5237 adds this to the classpath for the tests in iceberg-api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants