[zorg] OpenMPBuilder: Fix property substitution for lit arguments

[Hahnfeld]

Summary

The result of WithProperties needs to be rendered for a particular
build. That is why the current string conversion doesn't work.
Solve the problem by deferring the call to WithProperties until
we have built the complete argument string so that the returned
value can be passed as-is with the command array.

[Hahnfeld]

Ping. Can we get this in soon to finally fix the openmp bots?

[Hahnfeld]

@gkistanova Could you take a look please?

[gkistanova]

Hello Jonas.
This approach does not support a build slave "jobs" property, making it hard to support multiple slaves for the same builder.
However, since this change is local for OpenMPBuilder, you may go ahead and commit it, if you are not up to a better implementation at the moment.

[Hahnfeld]

In D39690#930874, @gkistanova wrote:

Hello Jonas.
This approach does not support a build slave "jobs" property, making it hard to support multiple slaves for the same builder.
However, since this change is local for OpenMPBuilder, you may go ahead and commit it, if you are not up to a better implementation at the moment.

Yes, I also don't like that. But I couldn't find another way than WithProperties which currently does not work because it is rendered for a particular build... If you another idea, I'm happy to implement that.

[Hahnfeld]

Use a different approach which allows to reuse the builder property.

[gkistanova]

WithProperties should work just fine in this context.
It gets rendered for each build, but will always be the same, unless someone would overwrite it for a particular build.

I'm thinking of handling this in CmakeCommand and LitTestCommand transparently for the factories. Basically, what we want is to enforce the jobs param to lit the same way it gets handled for ninja.
But again, this is a larger scope than what you have in mind for this patch.

[Hahnfeld]

In D39690#930881, @Hahnfeld wrote:

In D39690#930874, @gkistanova wrote:

Hello Jonas.
This approach does not support a build slave "jobs" property, making it hard to support multiple slaves for the same builder.
However, since this change is local for OpenMPBuilder, you may go ahead and commit it, if you are not up to a better implementation at the moment.

Yes, I also don't like that. But I couldn't find another way than WithProperties which currently does not work because it is rendered for a particular build... If you another idea, I'm happy to implement that.

I requestioned why the current code didn't work and came to the conclusion that the problem must be a bit different if you look into the details: The assumption is that WithProperties works in general, also when passed in the command array, which many other builders do. So the only thing that can possibly be wrong is the implicit string conversion when the result from WithProperties was formatted as %s. If that is true, the updated patch should fix the problem by deferring the rendering and reusing the "jobs" property. Do you agree on my analysis?

[Hahnfeld]

I'm going to commit this patch (which is now rather a fix for D39070) so that it can be deployed during the scheduled restart of the buildbot master.

[Hahnfeld]

This has worked (partly), the arguments are correctly rendered now. Next problem: lit complains about the quotes that are apparently not needed for the Configure() command. I've fixed this in rL318762 and now need to wait until the master is updated (again). Sorry for the long list of fixes for this change :-(

zorg/trunk/zorg/buildbot/builders/OpenMPBuilder.py

def getOpenMPCMakeBuildFactory(
     cmake_args += ['-DCMAKE_C_COMPILER=%s' % c_compiler]
     cmake_args += ['-DCMAKE_CXX_COMPILER=%s' % cxx_compiler]
     if libomptarget:
         cmake_args += ['-DOPENMP_ENABLE_LIBOMPTARGET=ON']
     if ompt:
         cmake_args += ['-DLIBOMP_OMPT_SUPPORT=ON']
 
     if test:
-        lit_args = WithProperties('-v --show-unsupported --show-xfail -j %s' % jobs)
-        cmake_args += ['-DLIBOMP_LIT_ARGS="%s"' % lit_args]
+        lit_args = '-v --show-unsupported --show-xfail -j %s' % jobs
+        cmake_args += [WithProperties('-DLIBOMP_LIT_ARGS="%s"' % lit_args)]
         if test_libomptarget:
-            cmake_args += ['-DLIBOMPTARGET_LIT_ARGS="%s"' % lit_args]
+            cmake_args += [WithProperties('-DLIBOMPTARGET_LIT_ARGS="%s"' % lit_args)]
 
     f.addStep(
         Configure(
             name        = 'configure-openmp',
             command     = cmake_args,
             description = 'configure openmp',
             workdir     = openmp_builddir,
             env         = merged_env))