This is an archive of the discontinued LLVM Phabricator instance.

[zorg] Rev1: Fix get slave environment in LLDB Windows builder.
ClosedPublic

Authored by rfoos on Jan 20 2015, 5:44 PM.

Details

Summary

Fixed RemoveDirectory, regenerated patch.

Diff Detail

Event Timeline

rfoos updated this revision to Diff 18471.Jan 20 2015, 5:44 PM
rfoos retitled this revision from to [zorg] Rev1: Fix get slave environment in LLDB Windows builder..
rfoos updated this object.
rfoos edited the test plan for this revision. (Show Details)
rfoos added a reviewer: gkistanova.
rfoos set the repository for this revision to rL LLVM.
rfoos added a project: Restricted Project.
rfoos added a subscriber: Unknown Object (MLST).
rfoos added a subscriber: zturner.

Thanks a million times for working on this. I never would have figured out the part of about the slave environment on my own :)

zorg/buildbot/builders/LLDBBuilder.py
37

I'm not crazy about this %VS120COMNTOOLS% usage. This is going to break when we switch to a different version of Visual Studio. In getLLDBWindowsCMakeBuildFactory(), we're already specifying a path to the Visual Studio installation folder, so I think we should just pass that into this function. This way, one can easily make two builders with different versions of Visual Studio on the same slave by just specifying a different value for vs_install_dir when creating the factory.

53

While it will probably work either way, any reason why this was necessary?

74

Can you move the comment inside the function?

127–132

Is the install necessary? What happens if we don't install? I've never done it before, so it seems like an unnecessary step unless I'm missing something.

134–140

Can getLLDBWindowsCMAKEBuildFactory take a boolean that says whether to run tests?

rfoos planned changes to this revision.Jan 21 2015, 9:16 AM

The changes are reasonably small. I'll get this back up, and would like to go live soon.

zorg/buildbot/builders/LLDBBuilder.py
37

Yes, this is temporary. I was just minimizing changes to your builder.

In the generic builder, slave_envCmd lets the caller specify a complete command that is run on the slave. This will handle any case.
http://reviews.llvm.org/D6866

r""""%VS120COMNTOOLS%\..\..\VC\vcvarsall.bat" %PROCESSOR_ARCHITECTURE% & set"""

I picked VS120COMNTOOLS since it handles multiple installations of VS. For example on my buildlsave, three versions of MSVC are installed. All can access vcvarsall.bat in the same way by changing the VSxxxCOMNTOOLS variable.

In this way, it will work on your slave or my slave as a default. Since the entire command can be specified by the caller, any special cases can be easily addressed.

VS100COMNTOOLS=c:\Program Files (x86)\Microsoft Visual Studio 10.0\Common7\Tools
VS110COMNTOOLS=C:\Program Files (x86)\Microsoft Visual Studio 11.0\Common7\Tools
VS120COMNTOOLS=C:\Program Files (x86)\Microsoft Visual Studio 12.0\Common7\Tools
53

Yes both ways work. All of the additions to python_source_dir were forward slash paths, so the result was a mixed path. CMake can handle this. Make can most of the time.

Basically all forward slashes was just easier on my eyes when reading the log file :)

"-DPYTHON_LIBRARY=C:\src\python/PCbuild/python27_d.lib" 
"-DPYTHON_INCLUDE_DIR=C:\src\python/Include"
"-DPYTHON_EXECUTABLE=C:\src\python/PCbuild/python_d.exe"
74

Yes the comments can move. In the proposed new getLLVMSource, you would just add LLD as an additional entry to the project list

[svn-llvm, svn-clang, svn-lldb, svn-lld]
127–132

The current CMake install step doesn't copy over the python DLL, or site-packages, and there is no info inside the exe on where to find them.

Doing the install step exposes these problems. But you need to get the installed version from the buildslave manually to see there is something wrong.

In the future, adding the ability to run the installed version of LLDB through LIT would be useful.

134–140

Yes, but one of the goals of this build factory was to put in place additions so that LLDB could run tests on Windows.

zturner added inline comments.Jan 21 2015, 9:36 AM
zorg/buildbot/builders/LLDBBuilder.py
37

Ultimately I guess I just want one way of specifying the VS location. Currently it's specified twice (once when you create this factory, and once when this function is run), and it would be be possible for them to get out of sync. As long as the person creating the factory can point to a different VS location, and as long as whatever they specify gets plumbed all the way through, then it's fine.

134–140

Yes but right now most of the tests still fail and haven't yet been marked XFAIL. So it would be nice if we could leave the tests off until we get that sorted out, so that we can at least verify the tree remains green in the meantime. Otherwise the builder will just be red all the time. Also eventually we will have multiple bots, and we may want some bots that only test compilation with different settings, and then have one master configuration that runs tests. To save CPU cycles on the machine for example.

rfoos updated this revision to Diff 18538.Jan 21 2015, 11:15 AM
rfoos removed rL LLVM as the repository for this revision.

Moved comments to getLLDBSource.

Replaced vs_install_dir with vs_common, and allow caller to change the value. Compromise until entire slave env command passed by caller. Caller can select VS version.

Added test, and install step flags so caller can select the additional steps. Both are default true.

If test has a value of 'ignoreFail', a test step failure will not fail the build (current default behavior).

gkistanova edited edge metadata.Jan 21 2015, 11:59 AM

Thanks for working on this, Rick!

If I get it right, this will be refactored once D6866 is in place. With this in mind, the patch looks good to me, assuming you will make the tests optional to reduce the immediate noise of the tests false failures.

zorg/buildbot/builders/LLDBBuilder.py
127–132

I'm second on this. We should do the formal install and use the installed result for testing.
This would help exposing missing dependencies.
With few exceptions when the install takes too long or requires too much resources on a particular slave. Though, this is not a subject of this particular patch.

134–140

Not sure about boolean, but agree in general. It should be a graceful way to skip tests for a particular builder.

rfoos accepted this revision.Jan 21 2015, 12:35 PM
rfoos added a reviewer: rfoos.

Ready to go.

zorg/buildbot/builders/LLDBBuilder.py
56

This allows VS versions to be selected by env variable or a hardcoded path to a place where ..\..\VC\vcvarsall.bat can be found.

I'd prefer when we refactor to allow the entire slave command to be specified here. This would replace all the function of generateVisualStudioEnvironment.

I've used the command to set a path to a 64 bit python in addition to the VS dev environment for a specific builder. A full command also leaves it compatible with linux as printenv works with this routine as well.

62

Both test and install can be disabled here.

Additionally, the test step can be marked ignoreFail'ures. This executes the tests, but ignores the failures.

All the builds will be green whether tests pass or fail. This is what we do here until experimental tests are working.

The defaults for test and install are on for now, and the builder can select if something different is needed.

This revision is now accepted and ready to land.Jan 21 2015, 12:35 PM
rfoos closed this revision.Jan 23 2015, 8:40 AM

r226700, and r226731

URL: http://llvm.org/viewvc/llvm-project?rev=226700&view=rev
Log: [zorg] Rev1: Fix get slave environment in LLDB Windows builder.

URL: http://llvm.org/viewvc/llvm-project?rev=226731&view=rev
Log: [zorg] Fix ignorefail in r226700