[BEAM-357] fixing build on windows #496
Conversation
|
R: @jbonofre |
|
Starting the review. |
| @@ -60,6 +60,9 @@ | |||
| <goals> | |||
| <goal>integration-test</goal> | |||
| </goals> | |||
| <configuration> | |||
| <ignoreEOLStyle>true</ignoreEOLStyle> <!-- for win --> | |||
There was a problem hiding this comment.
Can you remove the comment? Alternatively, please expand as to why it is needed, etc.
|
Hi Romain, On a high level -- we really need to work well on Windows. Thanks for starting this effort. In fact, we need to add integration testing on Windows as soon as possible. Code-wise, we need to find a different solution -- we cannot really download random, untrusted files and execute them on users' machine. Again, thanks and welcome! |
|
Hi Davor, Updated the comments. Let me know if it is better or worse ;). Changed a bit the downloading to add a sanity check (we could replace it by a checksum but the best would be to be able to use gpg --verify but seems bouncycastle doesn't fully match gpg format). This repo is not really random since it is provided by an asf committer and I downgraded to a signed binary version. A maybe better alternative is to package a beam-hadoop-win jar with this exe and just explode it if running tests on windows - drawback is we put a binary in a jar, advantage is beam owns this binary. wdyt? |
|
This is definitely safer, but I feel we are still exposing a potentially unnecessary attack surface to our users. Would you be able to summarize the problem on the Windows platform, and how does this binary solves it? |
|
Sure, on windows hadoop relies for most commands (including chmods which are the first commands triggered in beam) on winutils.exe to actually execute the action (https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java#L224 for instance). It does find the winutils.exe from hadoop home which is ATM not set in beam. If you have locally hadoop with winutils.exe - not provided by ASF yet - and set the home properly then most of tests - excepted the one using Path but that's something else - should be fine but if you don't you need that workaround cause Shell of hadoop get winutils in a static block (https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java#L639) and it can be null. If null it will not remove the option but execute commands with a null entry in the array (processbuilder.execute([null, "chmod", ....])) which leads to a NPE. Side note: if you are curious chmod is implemented like that https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/winutils/chmod.c with win API We can also propose to hadoop a degraded mode where if winutils is not there and a "test" flag activated we try to do these operations in java and if it fails we ignore them when acceptable (chmod/chown should be ok to ignore for tests for instance). Didn't check if it would be doable for all winutils operations. |
|
I think the alternative could be a better approach overall. In general, There are others too -- tools like Cygwin, etc. provide many of these utilities. Finally, Microsoft announced Windows Subsystem for Linux (WSL) that enables the scenario too. So, I think we could try with a degraded mode first. What do you think? Separately, I have to kindly ask not to push commits to our repository, including creating a feature branch. |
|
Yep, cygwin could be detected and surely enough. Will try to find some time for it and finally change this PR for a hadoop upgrade + the few other fixes. About the push: mea culpa, bad ctrl+R, deleted it just after. |
|
(Also, feel free to separate into multiple PR. A good portion of this one can probably go in separately and is non-controversial. Of course, do as you see fit.) |
|
Rebasing and checking ... |
|
Should we close this if it is not active, and track it in JIRA instead, which can link back to the closed PR? |
|
ok with me |
|
+1 to close |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 1.08 MB...] at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Archetype IT 'basic' failed: Execution failure: exit code = 1 at org.apache.maven.archetype.mojos.IntegrationTestMojo.execute(IntegrationTestMojo.java:269) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-02-27T18:32:16.636 [ERROR] 2017-02-27T18:32:16.636 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-27T18:32:16.637 [ERROR] 2017-02-27T18:32:16.637 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-27T18:32:16.637 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-02-27T18:32:16.637 [ERROR] 2017-02-27T18:32:16.637 [ERROR] After correcting the problems, you can resume the build with the command2017-02-27T18:32:16.637 [ERROR] mvn -rf :beam-sdks-java-maven-archetypes-starterchannel stoppedSetting status of 460d21c to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7896/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
|
@rmannibucau, can you close please then? |
|
yep :) |
PiperOrigin-RevId: 483808940
) Closes apache#421. Supersedes apache#496.
Small fixes or workarounds for windows.
Main issues were: