[PATCH] D29677: [buildbot] Add check-fuzzer to Asan buildbot on Windows.

[Galina via Phabricator via llvm-commits]

gkistanova requested changes to this revision.
gkistanova added a comment.
This revision now requires changes to proceed.

Hello Marcos,

I have commented couple things inline.

You may want to reconsider how you try to set the environments. What you are trying to do wouldn't work. `Property('slave_env')` is a local snapshot of the slave envs, and does not get propagated back to the slave.
You either would need to use a build step `env` param to set an env var, or do something else which would not require that.
At the first glance, Reid's suggestion of reducing the number of dependencies looks as a way to go, unless you have a strong reason to dial with DLLs.



================
Comment at: zorg/buildbot/builders/SanitizerBuilderWindows.py:117
+    # Clean fuzzer build dir.
+    f.addStep(RemoveDirectory(name='clean '+build_fuzzer_dir,
+                dir=build_fuzzer_dir,
----------------
Depending on a build slave version, RemoveDirectory might not work well on Windows, if some directories are not empty or temporarily locked by some service or anti-virus. Just FYI.

Unfortunately, there is no good solution for this. Issuing "rmdir /S /Q" command might fail too, depending on the shell.



================
Comment at: zorg/buildbot/builders/SanitizerBuilderWindows.py:132
+    # Get clang version.
+    lib_clang_dir = "\\".join([Property("builddir"), config, "lib", "clang"])
+    f.addStep(SetProperty(name="get clang version",
----------------
All these could be done simpler.

Every build step has a property "workdir" which contains a fully qualified path. When referencing a path, you could do something like

```
           WithProperties(
                "-DCMAKE_CXX_COMPILER=%(workdir)s/" + staged_install + "/bin/clang++"
            ))
```
No need to be canonical with the directory separator as well, forward slashes would do just fine.

You can check how similar to this is done in the UnifiedTreeBuilder.py, lines 365-375.


================
Comment at: zorg/buildbot/builders/SanitizerBuilderWindows.py:149
+    Property('slave_env')['Path'] = ";".join([bin_path, dll_path,
+        Property('slave_env')['Path']])
+
----------------
All these could be done simpler.

Every build step has a property "workdir" which contains a fully qualified path. When referencing a path, you could do something like

```
           WithProperties(
                "-DCMAKE_CXX_COMPILER=%(workdir)s/" + staged_install + "/bin/clang++"
            ))
```
No need to be canonical with the directory separator as well, forward slashes would do just fine.

You can check how similar to this is done in the UnifiedTreeBuilder.py, lines 365-375.


================
Comment at: zorg/buildbot/builders/SanitizerBuilderWindows.py:151
+
+    f.addStep(ShellCommand(name='cmake',
+                           command=[cmake, "-G", "Ninja", "../llvm",
----------------
Could you use the `CmakeCommand` instead of the `ShellCommand`, please?



https://reviews.llvm.org/D29677