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

Update google-cloud-nio to support underscores in bucket names #1903

Merged
merged 1 commit into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
48 changes: 38 additions & 10 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ plugins {
id 'jacoco'
id 'application'
id 'com.palantir.git-version' version '0.5.1'
id 'com.github.johnrengelman.shadow' version '5.1.0'
id 'com.github.johnrengelman.shadow' version '8.1.1'
id "com.github.kt3k.coveralls" version '2.6.3'
id "org.ajoberstar.grgit" version "5.2.0"
id "org.ajoberstar.git-publish" version "2.1.1"
}

mainClassName = "picard.cmdline.PicardCommandLine"
application {
mainClass = "picard.cmdline.PicardCommandLine"
}

repositories {
mavenCentral()
Expand Down Expand Up @@ -62,7 +64,7 @@ dependencies {
implementation('com.intel.gkl:gkl:0.8.8') {
exclude module: 'htsjdk'
}
implementation 'com.google.guava:guava:15.0'
implementation 'com.google.guava:guava:32.1.1-jre'
implementation 'org.apache.commons:commons-math3:3.5'
implementation 'org.apache.commons:commons-collections4:4.3'
implementation 'com.github.samtools:htsjdk:' + htsjdkVersion
Expand All @@ -71,7 +73,7 @@ dependencies {
implementation 'org.apache.logging.log4j:log4j-core:2.17.1'
implementation 'org.openjdk.nashorn:nashorn-core:15.4'
implementation 'org.apache.commons:commons-lang3:3.6'
implementation 'com.google.cloud:google-cloud-nio:0.123.25'
implementation 'com.google.cloud:google-cloud-nio:0.127.0'

testImplementation 'org.testng:testng:6.14.3'
}
Expand All @@ -83,8 +85,10 @@ configurations.all {
}
}

sourceCompatibility = 1.17
targetCompatibility = 1.17
java {
sourceCompatibility = 1.17
targetCompatibility = 1.17
}

final isRelease = Boolean.getBoolean("release")
final gitVersion = gitVersion().replaceAll(".dirty", "")
Expand All @@ -95,6 +99,22 @@ group = 'com.github.broadinstitute'

defaultTasks 'all'

distZip {
dependsOn 'currentJar'
}

distTar {
dependsOn 'currentJar'
}

startScripts {
dependsOn 'currentJar'
}

startShadowScripts {
dependsOn 'currentJar'
}

task all(dependsOn: ['jar', 'distZip', 'javadoc', 'shadowJar', 'currentJar', 'picardDoc'])

// Source file names for the picard command line properties file. We select and include only one of
Expand Down Expand Up @@ -161,6 +181,10 @@ task picardDoc(type: Javadoc, dependsOn: ['cleanPicardDoc', classes]) {
options.addStringOption("absolute-version", getVersion())
options.addStringOption("build-timestamp", new Date().format("dd-mm-yyyy hh:mm:ss"))
options.addStringOption("verbose")
// Avoid 'javadoc: error - invalid flag: -notimestamp'. See:
// - https://github.com/gradle/gradle/issues/11898
// - https://github.com/gradle/gradle/issues/11898#issuecomment-993789034
options.noTimestamp(false)
}

task currentJar(type: Copy){
Expand Down Expand Up @@ -263,13 +287,13 @@ jacocoTestReport {
getAdditionalSourceDirs().from(sourceSets.main.allJava.srcDirs)

reports {
xml.enabled = true // coveralls plugin depends on xml format report
html.enabled = true
xml.required = true // coveralls plugin depends on xml format report
html.required = true
}
}

wrapper {
gradleVersion = '7.5.1'
gradleVersion = '8.2.1'
}

task javadocJar(type: Jar) {
Expand Down Expand Up @@ -362,7 +386,7 @@ publish {
ext.htmlDir = file("build/docs/html")

//update static web docs
task copyJavadoc(dependsOn: 'javadoc', type: Copy) {
task copyJavadoc(dependsOn: ['javadoc', 'picardDoc'], type: Copy) {
from 'build/docs/javadoc'
into "$htmlDir/javadoc"
}
Expand All @@ -388,3 +412,7 @@ gitPublish {
}
}
}

gitPublishCopy {
dependsOn 'updateGhPages', 'copyJavadoc', 'copyPicardDoc'
}
Binary file modified gradle/wrapper/gradle-wrapper.jar
Binary file not shown.
2 changes: 1 addition & 1 deletion gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.5.1-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-8.2.1-bin.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
6 changes: 6 additions & 0 deletions gradlew
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@ set -- \
org.gradle.wrapper.GradleWrapperMain \
"$@"

# Stop when "xargs" is not available.
if ! command -v xargs >/dev/null 2>&1
then
die "xargs is not available"
fi

# Use "xargs" to parse quoted args.
#
# With -n1 it outputs one arg per line, with the quotes and backslashes removed.
Expand Down
29 changes: 29 additions & 0 deletions src/test/java/picard/nio/GATKBucketUtilsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package picard.nio;

import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import org.testng.Assert;

import java.net.URI;
import java.nio.file.Path;
import java.nio.file.Paths;

public class GATKBucketUtilsTest {
Copy link
Member

Choose a reason for hiding this comment

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

Why move this tests here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied this here because I didn't see another test to catch if support for underscores in bucket names regressed.

I added the test after seeing "Added or modified tests to cover changes and any new functionality" in the checklist in the issues template.

Copy link
Member

Choose a reason for hiding this comment

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

Heh, right, that makes sense. I forgot why we were doing this in the first place...


@DataProvider
public Object[][] getVariousPathsForPrefetching(){
return new Object[][]{
{"file:///local/file", false},
{"gs://abucket/bucket", true},
{"gs://abucket_with_underscores", true},
};
}

@Test(groups="bucket", dataProvider = "getVariousPathsForPrefetching")
public void testIsEligibleForPrefetching(String path, boolean isPrefetchable){
final URI uri = URI.create(path);
final Path uriPath = Paths.get(uri);
Assert.assertEquals(GATKBucketUtils.isEligibleForPrefetching(uriPath), isPrefetchable);
}
}