damccorm commented on code in PR #24874:
URL: https://github.com/apache/beam/pull/24874#discussion_r1147997198
##########
playground/backend/internal/fs_tool/java_fs.go:
##########
@@ -30,19 +33,23 @@ import (
)
const (
- JavaSourceFileExtension = ".java"
- javaCompiledFileExtension = ".class"
+ JavaSourceFileExtension = ".java"
+ javaCompiledFileExtension = ".class"
+ javaEntryPointFullName = "public static void
main(java.lang.String[])"
+ javaDecompilerCommand = "javap"
+ juintRunWithTestAnnotationConstant = "Lorg/junit/runner/RunWith;"
)
// newJavaLifeCycle creates LifeCycle with java SDK environment.
func newJavaLifeCycle(pipelineId uuid.UUID, pipelinesFolder string) *LifeCycle
{
javaLifeCycle := newCompilingLifeCycle(pipelineId, pipelinesFolder,
JavaSourceFileExtension, javaCompiledFileExtension)
- javaLifeCycle.Paths.ExecutableName = executableName
+ javaLifeCycle.Paths.FindExecutableName = findExecutableName
+ javaLifeCycle.Paths.FindTestExecutableName = findTestExecutableName
return javaLifeCycle
}
-// executableName returns name that should be executed (HelloWorld for
HelloWorld.class for java SDK)
-func executableName(executableFileFolderPath string) (string, error) {
+// findExecutableName returns name of .class file which has main() method
Review Comment:
```suggestion
// findExecutableName returns name of the .class file which has main() method
```
##########
playground/backend/internal/fs_tool/java_fs.go:
##########
@@ -52,27 +59,112 @@ func executableName(executableFileFolderPath string)
(string, error) {
}
if len(dirEntries) == 1 {
- return strings.Split(dirEntries[0].Name(), ".")[0], nil
+ return utils.TrimExtension(dirEntries[0].Name()), nil
}
for _, entry := range dirEntries {
- content, err := ioutil.ReadFile(fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name()))
- if err != nil {
- logger.Error(fmt.Sprintf("error during file reading:
%s", err.Error()))
- break
- }
- ext := strings.Split(entry.Name(), ".")[1]
- sdk := utils.ToSDKFromExt("." + ext)
+ select {
+ case <-ctx.Done():
+ return "", ctx.Err()
+ default:
+ filePath := fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name())
+ content, err := os.ReadFile(filePath)
+ if err != nil {
+ logger.Error(fmt.Sprintf("error during file
reading: %s", err.Error()))
+ break
+ }
+ ext := filepath.Ext(entry.Name())
+ filename := strings.TrimSuffix(entry.Name(), ext)
+ sdk := utils.ToSDKFromExt(ext)
+
+ if sdk == pb.Sdk_SDK_UNSPECIFIED {
+ logger.Error("invalid file extension")
+ break
+ }
- if sdk == pb.Sdk_SDK_UNSPECIFIED {
- logger.Error("invalid a file extension")
- break
+ switch ext {
+ case javaCompiledFileExtension:
+ isMain, err := isMainClass(ctx,
executableFileFolderPath, filename)
+ if err != nil {
+ return "", err
+ }
+ if isMain {
+ logger.Infof("executableName(): main
file is %s", filename)
+ return filename, nil
+ }
+ default:
+ if utils.IsFileMain(string(content), sdk) {
+ return filename, nil
+ }
+ }
}
+ }
+
+ return "", errors.New("cannot find file with main() method")
+}
+
+// findTestExecutableName returns name of .class file which has JUnit tests
+func findTestExecutableName(ctx context.Context, executableFileFolderPath
string) (string, error) {
+ dirEntries, err := os.ReadDir(executableFileFolderPath)
+ if err != nil {
+ return "", err
+ }
+ if len(dirEntries) < 1 {
+ return "", errors.New("number of executable files should be at
least one")
+ }
+
+ if len(dirEntries) == 1 {
+ return utils.TrimExtension(dirEntries[0].Name()), nil
+ }
+
+ for _, entry := range dirEntries {
+ select {
+ case <-ctx.Done():
+ return "", ctx.Err()
+ default:
+ if err != nil {
Review Comment:
When would this condition trigger?
##########
playground/backend/internal/fs_tool/java_fs.go:
##########
@@ -52,27 +59,112 @@ func executableName(executableFileFolderPath string)
(string, error) {
}
if len(dirEntries) == 1 {
- return strings.Split(dirEntries[0].Name(), ".")[0], nil
+ return utils.TrimExtension(dirEntries[0].Name()), nil
}
for _, entry := range dirEntries {
- content, err := ioutil.ReadFile(fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name()))
- if err != nil {
- logger.Error(fmt.Sprintf("error during file reading:
%s", err.Error()))
- break
- }
- ext := strings.Split(entry.Name(), ".")[1]
- sdk := utils.ToSDKFromExt("." + ext)
+ select {
+ case <-ctx.Done():
+ return "", ctx.Err()
+ default:
+ filePath := fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name())
+ content, err := os.ReadFile(filePath)
+ if err != nil {
+ logger.Error(fmt.Sprintf("error during file
reading: %s", err.Error()))
+ break
+ }
+ ext := filepath.Ext(entry.Name())
+ filename := strings.TrimSuffix(entry.Name(), ext)
+ sdk := utils.ToSDKFromExt(ext)
+
+ if sdk == pb.Sdk_SDK_UNSPECIFIED {
+ logger.Error("invalid file extension")
+ break
+ }
- if sdk == pb.Sdk_SDK_UNSPECIFIED {
- logger.Error("invalid a file extension")
- break
+ switch ext {
+ case javaCompiledFileExtension:
+ isMain, err := isMainClass(ctx,
executableFileFolderPath, filename)
+ if err != nil {
+ return "", err
Review Comment:
Should we just log and break here like we do elsewhere in this function?
##########
playground/backend/internal/fs_tool/java_fs.go:
##########
@@ -52,27 +59,112 @@ func executableName(executableFileFolderPath string)
(string, error) {
}
if len(dirEntries) == 1 {
- return strings.Split(dirEntries[0].Name(), ".")[0], nil
+ return utils.TrimExtension(dirEntries[0].Name()), nil
}
for _, entry := range dirEntries {
- content, err := ioutil.ReadFile(fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name()))
- if err != nil {
- logger.Error(fmt.Sprintf("error during file reading:
%s", err.Error()))
- break
- }
- ext := strings.Split(entry.Name(), ".")[1]
- sdk := utils.ToSDKFromExt("." + ext)
+ select {
+ case <-ctx.Done():
+ return "", ctx.Err()
+ default:
+ filePath := fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name())
+ content, err := os.ReadFile(filePath)
+ if err != nil {
+ logger.Error(fmt.Sprintf("error during file
reading: %s", err.Error()))
+ break
+ }
+ ext := filepath.Ext(entry.Name())
+ filename := strings.TrimSuffix(entry.Name(), ext)
+ sdk := utils.ToSDKFromExt(ext)
+
+ if sdk == pb.Sdk_SDK_UNSPECIFIED {
+ logger.Error("invalid file extension")
+ break
+ }
- if sdk == pb.Sdk_SDK_UNSPECIFIED {
- logger.Error("invalid a file extension")
- break
+ switch ext {
+ case javaCompiledFileExtension:
+ isMain, err := isMainClass(ctx,
executableFileFolderPath, filename)
+ if err != nil {
+ return "", err
+ }
+ if isMain {
+ logger.Infof("executableName(): main
file is %s", filename)
+ return filename, nil
+ }
+ default:
+ if utils.IsFileMain(string(content), sdk) {
+ return filename, nil
+ }
+ }
}
+ }
+
+ return "", errors.New("cannot find file with main() method")
+}
+
+// findTestExecutableName returns name of .class file which has JUnit tests
+func findTestExecutableName(ctx context.Context, executableFileFolderPath
string) (string, error) {
+ dirEntries, err := os.ReadDir(executableFileFolderPath)
+ if err != nil {
+ return "", err
+ }
+ if len(dirEntries) < 1 {
+ return "", errors.New("number of executable files should be at
least one")
+ }
+
+ if len(dirEntries) == 1 {
+ return utils.TrimExtension(dirEntries[0].Name()), nil
+ }
+
+ for _, entry := range dirEntries {
+ select {
+ case <-ctx.Done():
+ return "", ctx.Err()
+ default:
+ if err != nil {
Review Comment:
I think never
##########
playground/backend/internal/fs_tool/java_fs.go:
##########
@@ -52,27 +59,112 @@ func executableName(executableFileFolderPath string)
(string, error) {
}
if len(dirEntries) == 1 {
- return strings.Split(dirEntries[0].Name(), ".")[0], nil
+ return utils.TrimExtension(dirEntries[0].Name()), nil
}
for _, entry := range dirEntries {
- content, err := ioutil.ReadFile(fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name()))
- if err != nil {
- logger.Error(fmt.Sprintf("error during file reading:
%s", err.Error()))
- break
- }
- ext := strings.Split(entry.Name(), ".")[1]
- sdk := utils.ToSDKFromExt("." + ext)
+ select {
+ case <-ctx.Done():
+ return "", ctx.Err()
+ default:
+ filePath := fmt.Sprintf("%s/%s",
executableFileFolderPath, entry.Name())
+ content, err := os.ReadFile(filePath)
+ if err != nil {
+ logger.Error(fmt.Sprintf("error during file
reading: %s", err.Error()))
+ break
+ }
+ ext := filepath.Ext(entry.Name())
+ filename := strings.TrimSuffix(entry.Name(), ext)
+ sdk := utils.ToSDKFromExt(ext)
+
+ if sdk == pb.Sdk_SDK_UNSPECIFIED {
+ logger.Error("invalid file extension")
+ break
+ }
- if sdk == pb.Sdk_SDK_UNSPECIFIED {
- logger.Error("invalid a file extension")
- break
+ switch ext {
+ case javaCompiledFileExtension:
+ isMain, err := isMainClass(ctx,
executableFileFolderPath, filename)
+ if err != nil {
+ return "", err
+ }
+ if isMain {
+ logger.Infof("executableName(): main
file is %s", filename)
+ return filename, nil
+ }
+ default:
+ if utils.IsFileMain(string(content), sdk) {
+ return filename, nil
+ }
+ }
}
+ }
+
+ return "", errors.New("cannot find file with main() method")
+}
+
+// findTestExecutableName returns name of .class file which has JUnit tests
+func findTestExecutableName(ctx context.Context, executableFileFolderPath
string) (string, error) {
+ dirEntries, err := os.ReadDir(executableFileFolderPath)
+ if err != nil {
+ return "", err
+ }
+ if len(dirEntries) < 1 {
+ return "", errors.New("number of executable files should be at
least one")
+ }
+
+ if len(dirEntries) == 1 {
+ return utils.TrimExtension(dirEntries[0].Name()), nil
+ }
+
+ for _, entry := range dirEntries {
+ select {
+ case <-ctx.Done():
+ return "", ctx.Err()
+ default:
+ if err != nil {
+ logger.Error(fmt.Sprintf("error during file
reading: %s", err.Error()))
+ break
+ }
+ ext := filepath.Ext(entry.Name())
+ filename := strings.TrimSuffix(entry.Name(), ext)
- if utils.IsFileMain(string(content), sdk) {
- return strings.Split(entry.Name(), ".")[0], nil
+ if ext == javaCompiledFileExtension {
+ isMain, err := isTestClass(ctx,
executableFileFolderPath, filename)
Review Comment:
Instead of `isMain`, should this be `isTest`?
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -142,36 +184,159 @@ func Test_executableName(t *testing.T) {
// Test case with calling sourceFileName method with
multiple files where one of them is main
// As a result, want to receive a name that should be
executed
name: "Multiple files where one of them is main",
- prepare: func() {
+ prepare: func() error {
compiled := filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), compiledFolderName)
- secondaryFilePath := filepath.Join(compiled,
"temp.scala")
- err := os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
+ primaryFilePath := filepath.Join(compiled,
"main.scala")
+ err := os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
if err != nil {
- panic(err)
+ return err
}
- primaryFilePath := filepath.Join(compiled,
"main.scala")
- err = os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
+ secondaryFilePath := filepath.Join(compiled,
"temp.scala")
+ err = os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
if err != nil {
- panic(err)
+ return err
}
+ return nil
},
+ cleanup: cleanupFunc,
args: args{
executableFolder: filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), "bin"),
},
want: "main",
wantErr: false,
},
+ {
+ // Test case with calling sourceFileName method with
multiple files where one of them is a .class file
+ // with main() method
+ // As a result, want to receive a name that should be
executed
+ name: "Multiple Java class files where one of them
contains main",
+ prepare: func() error {
+ testdataPath := "java_testdata"
+ sourceFile := filepath.Join(testdataPath,
"HasMainTest1.java")
+
+ err := compileJavaFiles(sourceFile)
Review Comment:
The test description says "Multiple Java class files" - should we also be
passing in other files here? Same question applies elsewhere
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -142,36 +184,159 @@ func Test_executableName(t *testing.T) {
// Test case with calling sourceFileName method with
multiple files where one of them is main
// As a result, want to receive a name that should be
executed
name: "Multiple files where one of them is main",
- prepare: func() {
+ prepare: func() error {
compiled := filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), compiledFolderName)
- secondaryFilePath := filepath.Join(compiled,
"temp.scala")
- err := os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
+ primaryFilePath := filepath.Join(compiled,
"main.scala")
+ err := os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
if err != nil {
- panic(err)
+ return err
}
- primaryFilePath := filepath.Join(compiled,
"main.scala")
- err = os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
+ secondaryFilePath := filepath.Join(compiled,
"temp.scala")
+ err = os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
if err != nil {
- panic(err)
+ return err
}
+ return nil
},
+ cleanup: cleanupFunc,
args: args{
executableFolder: filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), "bin"),
},
want: "main",
wantErr: false,
},
+ {
+ // Test case with calling sourceFileName method with
multiple files where one of them is a .class file
+ // with main() method
+ // As a result, want to receive a name that should be
executed
+ name: "Multiple Java class files where one of them
contains main",
+ prepare: func() error {
+ testdataPath := "java_testdata"
+ sourceFile := filepath.Join(testdataPath,
"HasMainTest1.java")
+
+ err := compileJavaFiles(sourceFile)
Review Comment:
(e.g. maybe HasNoMain.java)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
