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

feat: write samples to file (pt3) #980

Merged
merged 21 commits into from
Apr 28, 2022
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
71 changes: 70 additions & 1 deletion rules_java_gapic/java_gapic.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def _java_gapic_postprocess_srcjar_impl(ctx):
output_srcjar_name = ctx.label.name
output_main = ctx.outputs.main
output_test = ctx.outputs.test
output_samples = ctx.outputs.samples
output_resource_name = ctx.outputs.resource_name
formatter = ctx.executable.formatter

Expand Down Expand Up @@ -58,11 +59,16 @@ def _java_gapic_postprocess_srcjar_impl(ctx):
cd $WORKING_DIR/{output_dir_path}/src/test/java
zip -r $WORKING_DIR/{output_srcjar_name}-tests.srcjar ./

# Sample source files.
cd $WORKING_DIR/{output_dir_path}/samples/snippets/generated/src/main/java
zip -r $WORKING_DIR/{output_srcjar_name}-samples.srcjar ./

cd $WORKING_DIR

mv {output_srcjar_name}.srcjar {output_main}
mv {output_srcjar_name}-resource-name.srcjar {output_resource_name}
mv {output_srcjar_name}-tests.srcjar {output_test}
mv {output_srcjar_name}-samples.srcjar {output_samples}
""".format(
gapic_srcjar = gapic_srcjar.path,
output_srcjar_name = output_srcjar_name,
Expand All @@ -72,13 +78,14 @@ def _java_gapic_postprocess_srcjar_impl(ctx):
output_main = output_main.path,
output_resource_name = output_resource_name.path,
output_test = output_test.path,
output_samples = output_samples.path,
)

ctx.actions.run_shell(
inputs = [gapic_srcjar],
tools = [formatter],
command = script,
outputs = [output_main, output_resource_name, output_test],
outputs = [output_main, output_resource_name, output_test, output_samples],
)

_java_gapic_postprocess_srcjar = rule(
Expand All @@ -94,10 +101,66 @@ _java_gapic_postprocess_srcjar = rule(
"main": "%{name}.srcjar",
"resource_name": "%{name}-resource-name.srcjar",
"test": "%{name}-test.srcjar",
"samples": "%{name}-samples.srcjar",
},
implementation = _java_gapic_postprocess_srcjar_impl,
)

def _java_gapic_samples_srcjar_impl(ctx):
gapic_srcjar = ctx.file.gapic_srcjar
output_srcjar_name = ctx.label.name
output_samples = ctx.outputs.samples
formatter = ctx.executable.formatter

output_dir_name = ctx.label.name
output_dir_path = "%s/%s" % (output_samples.dirname, output_dir_name)

script = """
unzip -q {gapic_srcjar}
# Sync'd to the output file name in Writer.java.
unzip -q temp-codegen.srcjar -d {output_dir_path}
# This may fail if there are spaces and/or too many files (exceed max length of command length).
{formatter} --replace $(find {output_dir_path} -type f -printf "%p ")
WORKING_DIR=`pwd`

# Sample source files.
cd $WORKING_DIR/{output_dir_path}/samples/snippets/generated/src/main/java
zip -r $WORKING_DIR/{output_srcjar_name}-samples.srcjar ./

cd $WORKING_DIR

mv {output_srcjar_name}-samples.srcjar {output_samples}
""".format(
gapic_srcjar = gapic_srcjar.path,
output_srcjar_name = output_srcjar_name,
formatter = formatter,
output_dir_name = output_dir_name,
output_dir_path = output_dir_path,
output_samples = output_samples.path,
)

ctx.actions.run_shell(
inputs = [gapic_srcjar],
tools = [formatter],
command = script,
outputs = [output_samples],
)

_java_gapic_samples_srcjar = rule(
attrs = {
"gapic_srcjar": attr.label(mandatory = True, allow_single_file = True),
"formatter": attr.label(
default = Label("//:google_java_format_binary"),
executable = True,
cfg = "host",
),
},
outputs = {
"samples": "%{name}-samples.srcjar",
},
implementation = _java_gapic_samples_srcjar_impl,
)

def _extract_common_proto_dep(dep):
return dep[dep.index("/"):] if "//google" in dep else dep

Expand Down Expand Up @@ -198,6 +261,12 @@ def java_gapic_library(
**kwargs
)

_java_gapic_samples_srcjar(
name = "%s_samples" % name,
gapic_srcjar = "%s.srcjar" % raw_srcjar_name,
**kwargs
)

resource_name_name = "%s_resource_name" % name
resource_name_deps = [resource_name_name]
native.java_library(
Expand Down
28 changes: 25 additions & 3 deletions rules_java_gapic/java_gapic_pkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,29 @@ def _gapic_pkg_tar_impl(ctx):
for f in dep.files.to_list():
deps.append(f)

samples =[]
for s in ctx.attr.samples:
for f in s.files.to_list():
samples.append(f)

paths = _construct_package_dir_paths(
ctx.attr.package_dir,
ctx.outputs.pkg,
ctx.label.name,
)

script = """
for s in {samples}; do
mkdir -p {package_dir_path}/{tar_cd_suffix}/{tar_prefix}/samples/snippets/generated/
unzip -q ./$s -d {package_dir_path}/{tar_cd_suffix}/{tar_prefix}/samples/snippets/generated/
done

mkdir -p {package_dir_path}
for dep in {deps}; do
tar -xzpf $dep -C {package_dir_path}
done
cd {package_dir_path}/{tar_cd_suffix}

tar -zchpf {tar_prefix}/{package_dir}.tar.gz {tar_prefix}/*
cd -
mv {package_dir_path}/{package_dir}.tar.gz {pkg}
Expand All @@ -83,13 +94,14 @@ def _gapic_pkg_tar_impl(ctx):
deps = " ".join(["'%s'" % d.path for d in deps]),
package_dir_path = paths.package_dir_path,
package_dir = paths.package_dir,
samples = " ".join(["'%s'" % s.path for s in samples]),
pkg = ctx.outputs.pkg.path,
tar_cd_suffix = paths.tar_cd_suffix,
tar_prefix = paths.tar_prefix,
)

ctx.actions.run_shell(
inputs = deps,
inputs = deps + samples,
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
command = script,
outputs = [ctx.outputs.pkg],
)
Expand All @@ -101,6 +113,7 @@ def _gapic_pkg_tar_impl(ctx):
gapic_pkg_tar = rule(
attrs = {
"deps": attr.label_list(mandatory = True),
"samples": attr.label_list(mandatory = False),
"package_dir": attr.string(mandatory = False, default = ""),
"extension": attr.string(mandatory = False, default = "tar.gz"),
},
Expand Down Expand Up @@ -256,6 +269,10 @@ def _java_gapic_srcs_pkg_impl(ctx):
# Remove empty files. If there are no resource names, one such file might have
# been created. See java_gapic.bzl.
rm $(find {package_dir_path}/src/main/java -size 0)

if [ -d {package_dir_path}/src/main/java/samples ]; then
mv {package_dir_path}/src/main/java/samples {package_dir_path}
fi
done
for proto_src in {proto_srcs}; do
mkdir -p {package_dir_path}/src/main/proto
Expand Down Expand Up @@ -319,11 +336,14 @@ def java_gapic_assembly_gradle_pkg(
client_test_deps = []
grpc_deps = []
proto_deps = []
samples = []

processed_deps = {} #there is no proper Set in Starlark
for dep in deps:
# Use contains instead of endswith since microgenerator testing may use differently-named targets.
if "_java_gapic" in dep:
if "samples" in dep:
samples.append(dep)
elif "_java_gapic" in dep:
_put_dep_in_a_bucket(dep, client_deps, processed_deps)
_put_dep_in_a_bucket("%s_test" % dep, client_test_deps, processed_deps)
_put_dep_in_a_bucket("%s_resource_name" % dep, proto_deps, processed_deps)
Expand Down Expand Up @@ -371,6 +391,7 @@ def java_gapic_assembly_gradle_pkg(
name = name,
assembly_name = package_dir,
deps = proto_target_dep + grpc_target_dep + client_target_dep,
samples = samples,
)

def _java_gapic_gradle_pkg(
Expand Down Expand Up @@ -416,7 +437,7 @@ def _java_gapic_gradle_pkg(
**kwargs
)

def _java_gapic_assembly_gradle_pkg(name, assembly_name, deps, visibility = None):
def _java_gapic_assembly_gradle_pkg(name, assembly_name, deps, samples = None, visibility = None):
resource_target_name = "%s-resources" % assembly_name
java_gapic_build_configs_pkg(
name = resource_target_name,
Expand All @@ -434,6 +455,7 @@ def _java_gapic_assembly_gradle_pkg(name, assembly_name, deps, visibility = None
Label("//rules_java_gapic:gradlew"),
resource_target_name,
] + deps,
samples = samples,
package_dir = assembly_name,
visibility = visibility,
)
8 changes: 6 additions & 2 deletions scripts/diff_gen_and_golden.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ RAW_SRCJAR=$( find . -name '*_java_gapic_srcjar_raw.srcjar' )
mkdir unpacked src
cd unpacked
unzip -q -c "../${RAW_SRCJAR}" temp-codegen.srcjar | jar x

mkdir -p ../samples/snippets/generated/
cp -r samples/snippets/generated/**/* ../samples/snippets/generated

cp -r src/main/java/* ../src
cp -r src/test/java/* ../src
[ -d proto ] && cp -r proto/src/main/java/* ../src
Expand All @@ -17,7 +21,7 @@ cd ..
find src -type f ! -name '*.java' -a ! -name '*gapic_metadata.json' -delete
find src -type f -name 'PlaceholderFile.java' -delete
find src -type d -empty -delete

# This will not print diff_output to the console unless `--test_output=all` option
# is enabled, it only emits the comparison results to the test.log.
diff -ru src test/integration/goldens/${API_NAME}
diff -ru src test/integration/goldens/${API_NAME}/src
diff -ru samples test/integration/goldens/${API_NAME}/samples
10 changes: 7 additions & 3 deletions scripts/update_golden.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@ cd ${BUILD_WORKSPACE_DIRECTORY}/test/integration/goldens/${API_NAME}
find . -name '*.java' -delete
find . -name 'gapic_metadata.json' -delete

cp -r ${UNPACK_DIR}/src/main/java/* .
cp -r ${UNPACK_DIR}/src/test/java/* .
[ -d ${UNPACK_DIR}/proto ] && cp -r ${UNPACK_DIR}/proto/src/main/java/* .
mkdir -p ./src
cp -r ${UNPACK_DIR}/src/main/java/* ./src
cp -r ${UNPACK_DIR}/src/test/java/* ./src
[ -d ${UNPACK_DIR}/proto ] && cp -r ${UNPACK_DIR}/proto/src/main/java/* ./src

mkdir -p ./samples/snippets/generated
cp -r ${UNPACK_DIR}/samples/snippets/generated/**/* ./samples/snippets/generated

find . -name 'PlaceholderFile.java' -delete
find . -type d -empty -delete
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.GapicPackageInfo;
import com.google.api.generator.gapic.model.Sample;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.model.Transport;
import com.google.common.annotations.VisibleForTesting;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

Expand All @@ -45,7 +47,7 @@ public static List<GapicClass> composeServiceClasses(GapicContext context) {
clazzes.addAll(generateServiceClasses(context));
clazzes.addAll(generateMockClasses(context, context.mixinServices()));
clazzes.addAll(generateResourceNameHelperClasses(context));
return addApacheLicense(clazzes);
return addApacheLicense(composeSamples(clazzes, context.gapicMetadata().getProtoPackage()));
}

public static GapicPackageInfo composePackageInfo(GapicContext context) {
Expand Down Expand Up @@ -186,6 +188,40 @@ public static List<GapicClass> generateTestClasses(GapicContext context) {
return clazzes;
}

@VisibleForTesting
static List<GapicClass> composeSamples(List<GapicClass> clazzes, String protoPackage) {
Copy link
Collaborator

@blakeli0 blakeli0 Apr 26, 2022

Choose a reason for hiding this comment

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

This method is basically adding region tags and headers to all samples in all classes, which should probably be renamed as well. But before you do that, I have a basic question regarding why we want to do it here after samples are already mostly composed? The package info is already in the context which is passes into generateServiceClasses, so potentially we should be able to generate a full Sample in previous steps right?

Copy link
Contributor Author

@eaball35 eaball35 Apr 26, 2022

Choose a reason for hiding this comment

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

Before this step, we can generate an inline sample but not an "executable" one because inline format doesn't need a proper region tag or license. Looking at composeServiceClasses called in Generator before Writer I could parse shortname/version out before or start of composeServiceClasses() and keep passing these details around when creating Samples or I can add these details before writing the files as I'm doing. I already am iterating over the samples to add the license header here and these details aren't needed before this step.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's unfortunate these info are not easily accessible and you have to pass them around. I have some refactoring ideas but should definitely not be the scope of this PR. For now, can you please rename this method to make more readable/precise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense and I agree it's not ideal, assuming the behaviors won't change I can test this out overall and refactor in parallel.

done, I renamed to make more readable

// parse protoPackage for apiVersion and apiShortName
String[] pakkage = protoPackage.split("\\.");
String apiVersion;
String apiShortName;
// e.g. v1, v2, v1beta1
if (pakkage[pakkage.length - 1].matches("v[0-9].*")) {
apiVersion = pakkage[pakkage.length - 1];
apiShortName = pakkage[pakkage.length - 2];
} else {
apiVersion = "";
apiShortName = pakkage[pakkage.length - 1];
}
// Include license header, apiShortName, and apiVersion
return clazzes.stream()
.map(
gapicClass -> {
List<Sample> samples =
gapicClass.samples().stream()
.map(sample -> addRegionTagAndHeaderToSample(sample, apiShortName, apiVersion))
.collect(Collectors.toList());
return gapicClass.withSamples(samples);
})
.collect(Collectors.toList());
}

private static Sample addRegionTagAndHeaderToSample(Sample sample, String apiShortName, String apiVersion) {
return sample
.withHeader(Arrays.asList(CommentComposer.APACHE_LICENSE_COMMENT))
.withRegionTag(
sample.regionTag().withApiVersion(apiVersion).withApiShortName(apiShortName));
}

@VisibleForTesting
protected static List<GapicClass> addApacheLicense(List<GapicClass> gapicClassList) {
return gapicClassList.stream()
Expand All @@ -197,7 +233,7 @@ protected static List<GapicClass> addApacheLicense(List<GapicClass> gapicClassLi
.toBuilder()
.setFileHeader(CommentComposer.APACHE_LICENSE_COMMENT)
.build();
return GapicClass.create(gapicClass.kind(), classWithHeader);
return GapicClass.create(gapicClass.kind(), classWithHeader, gapicClass.samples());
})
.collect(Collectors.toList());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,4 @@ private static String generateSampleClassName(RegionTag regionTag) {
+ regionTag.rpcName()
+ regionTag.overloadDisambiguation();
}

public String generateSampleFileName() {
String name = (regionTag().isAsynchronous() ? "Async" : "Sync") + regionTag().rpcName();
if (!regionTag().overloadDisambiguation().isEmpty()) {
name += "_" + regionTag().overloadDisambiguation();
}
return name;
}
}
Loading