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.

Sign up for GitHub

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

Jump to bottom

Build: Add iceberg-build.properties to Jars and release process #5236

Merged
merged 3 commits into from
Jul 10, 2022
Merged

Build: Add iceberg-build.properties to Jars and release process #5236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
911fffe
Build: Add iceberg-build.properties to Jars and release process.
rdblue Jul 10, 2022
fa9b1f3
Build: Call generateGitProperties directly in source-release.sh.
rdblue Jul 10, 2022
c4aeebe
Generate iceberg-build.properties after tagging to include the tag name.
rdblue Jul 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

}
generateGitProperties.outputs.upToDateWhen { false }
} catch (Exception e) {
Expand Down
4 changes: 4 additions & 0 deletions deploy.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ subprojects {

// add LICENSE and NOTICE
[jar, sourceJar, javadocJar, testJar].each { task ->
task.dependsOn rootProject.tasks.buildInfo
task.from("${rootDir}/build") {
include 'iceberg-build.properties'
}
task.from(rootDir) {
include 'LICENSE'
include 'NOTICE'
Expand Down
17 changes: 11 additions & 6 deletions dev/source-release.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,7 @@ tagrc="${tag}-rc${rc}"

echo "Preparing source for $tagrc"

echo "Adding version.txt and tagging release..."
echo $version > $projectdir/version.txt
git add $projectdir/version.txt
git commit -m "Add version.txt for release $version" $projectdir/version.txt

echo "Creating release candidate tag: $tagrc..."
set_version_hash=`git rev-list HEAD 2> /dev/null | head -n 1 `
git tag -am "Apache Iceberg $version" $tagrc $set_version_hash

Expand All @@ -105,11 +101,20 @@ if [ -z "$release_hash" ]; then
exit
fi

echo "Generating version.txt and iceberg-build.properties..."
echo $version > $projectdir/version.txt
./gradlew generateGitProperties
cp $projectdir/build/iceberg-build.properties $projectdir/iceberg-build.properties

# be conservative and use the release hash, even though git produces the same
# archive (identical hashes) using the scm tag
echo "Creating tarball ${tarball} using commit $release_hash"
tarball=$tag.tar.gz
git archive $release_hash --worktree-attributes --prefix $tag/ -o $projectdir/$tarball
git archive $release_hash --worktree-attributes --prefix $tag/ --add-file $projectdir/version.txt --add-file $projectdir/iceberg-build.properties -o $projectdir/$tarball

# 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.


echo "Signing the tarball..."
[[ -n "$keyid" ]] && keyopt="-u $keyid"
Expand Down
12 changes: 12 additions & 0 deletions tasks.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,15 @@ task deploySite(type: Exec) {
workingDir 'site'
commandLine('./deploy.sh')
}

if (file("${rootDir}/iceberg-build.properties").exists()) {
tasks.register('buildInfo', Exec) {
project.logger.info('Using build info from iceberg-build.properties')
commandLine 'cp', "${rootDir}/iceberg-build.properties", 'build/iceberg-build.properties'
}
} else {
tasks.register('buildInfo') {
project.logger.info('Generating iceberg-build.properties from git')
dependsOn generateGitProperties
}
}