> On March 22, 2018, 6:29 p.m., Szabolcs Vasas wrote: > > Hi Anna, > > > > Thank you for the patch, it makes the contribution much easier! > > I will continue my review tomorrow, I just wanted to post the findings from > > today. > > > > 1) The gradle wrapper script does not find the gradle-wrapper.jar, can you > > please add it to the patch as well > > (https://docs.gradle.org/current/dsl/org.gradle.api.tasks.wrapper.Wrapper.html)? > > 2) Once the gradle-wrapper.jar is present the ./gradlew test runs > > successfully > > 3) The project can be imported to IntelliJ without any problems > > 4) I think a couple of more items should be added to .gitignore > > .gradle/ -> I think this is generated by the gradle wrapper > > out/ -> I am not sure why but when I import the project to IntelliJ and > > run a test from there it generates the out folder which was not present > > with the ant build > > 5) Can we define the sourceCompatibility in build.gradle to say that we > > support 1.7 currently? > > 6) I have executed the package target with both ant and gradle and I found > > some differences, can you please check that these are expected? > > In case of gradle build bat files are also generated for sqoop tools > > scripts. > > The .gradle folder is copied to > > build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0, I am not sure this is desired. > > Some library versions are different in case of ant and gradle build. > > > > I am posting the relevant lines from the diff output: > > Only in buildgradle/bin: sqoop-codegen.bat > > Only in buildgradle/bin: sqoop-create-hive-table.bat > > Only in buildgradle/bin: sqoop-eval.bat > > Only in buildgradle/bin: sqoop-export.bat > > Only in buildgradle/bin: sqoop-help.bat > > Only in buildgradle/bin: sqoop-import-all-tables.bat > > Only in buildgradle/bin: sqoop-import-mainframe.bat > > Only in buildgradle/bin: sqoop-import.bat > > Only in buildgradle/bin: sqoop-job.bat > > Only in buildgradle/bin: sqoop-list-databases.bat > > Only in buildgradle/bin: sqoop-list-tables.bat > > Only in buildgradle/bin: sqoop-merge.bat > > Only in buildgradle/bin: sqoop-metastore.bat > > Only in buildgradle/bin: sqoop-version.bat > > Only in build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1: > > file-changes > > Binary files > > build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/fileContent/fileContent.lock > > and > > buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/fileContent/fileContent.lock > > differ > > Binary files > > build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileHashes.bin > > and > > buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileHashes.bin > > differ > > Binary files > > build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileSnapshots.bin > > and > > buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileSnapshots.bin > > differ > > Binary files > > build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.bin > > and > > buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.bin > > differ > > Binary files > > build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.lock > > and > > buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.lock > > differ > > Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: > > avro-ipc-1.8.1.jar > > Only in build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: > > commons-codec-1.4.jar > > Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: > > commons-codec-1.9.jar > > Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: > > parquet-hive-bundle-1.6.0.jar > > Only in build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: > > slf4j-api-1.6.1.jar > > Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: > > slf4j-api-1.7.7.jar
Hi Szabi, 1) I've added that with my previous update 2) cool :) 3) cool cool :) 4) I will add it, as for the out folder, it doesn't generate that for me when running the test, not sure what that is 5) sure, will add! 6) The version differences are expected (it used to be hardcoded versions that were independent of the used version), but I agree the .gradle folder should not be copied. I'll update the patch based on this. - Anna ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66067/#review199660 ----------------------------------------------------------- On March 22, 2018, 6:32 p.m., Anna Szonyi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66067/ > ----------------------------------------------------------- > > (Updated March 22, 2018, 6:32 p.m.) > > > Review request for Sqoop. > > > Bugs: Sqoop-3052 > https://issues.apache.org/jira/browse/Sqoop-3052 > > > Repository: sqoop-trunk > > > Description > ------- > > SQOOP-3052: Introduce gradle based build for Sqoop to make it more developer > friendly / open > > > Diffs > ----- > > .gitignore 68cbe28 > COMPILING.txt 86be509 > build.gradle PRE-CREATION > buildSrc/customUnixStartScript.txt PRE-CREATION > buildSrc/customWindowsStartScript.txt PRE-CREATION > buildSrc/sqoop-package.gradle PRE-CREATION > buildSrc/sqoop-version-gen.gradle PRE-CREATION > config/checkstyle/checkstyle-java-header.txt PRE-CREATION > config/checkstyle/checkstyle-noframes.xsl PRE-CREATION > config/checkstyle/checkstyle.xml PRE-CREATION > gradle.properties PRE-CREATION > gradle/wrapper/gradle-wrapper.jar PRE-CREATION > gradle/wrapper/gradle-wrapper.properties PRE-CREATION > gradlew PRE-CREATION > gradlew.bat PRE-CREATION > settings.gradle PRE-CREATION > > > Diff: https://reviews.apache.org/r/66067/diff/2/ > > > Testing > ------- > > ran all new tasks, except for internal maven publishing > > Notes: > - To try it out you can call ./gradlew tasks --all to see all the tasks and > compare them to current tasks/artifacts. > - Replaced cobertura with jacoco, as it's easier/cleaner to configure, easier > to combine all test results into a single report. > - Generated pom.xml now has correct dependencies/versions > - Script generation is currently hardcoded and not based on sqoop help, as > previously - though added the possiblity of hooking it in later > > > Thanks, > > Anna Szonyi > >
