Page MenuHomePhabricator

[zorg] OpenMPBuilder: Fix property substitution for lit arguments
ClosedPublic

Authored by Hahnfeld on Nov 6 2017, 9:31 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Nov 6 2017, 9:31 AM

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

@gkistanova Could you take a look please?

gkistanova edited edge metadata.Nov 20 2017, 3:44 PM

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.

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 updated this revision to Diff 123684.Nov 20 2017, 4:20 PM
Hahnfeld retitled this revision from [zorg] Statically configure jobs for testing to [zorg] OpenMPBuilder: Fix property substitution for lit arguments.
Hahnfeld edited the summary of this revision. (Show Details)

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

gkistanova added a comment.EditedNov 20 2017, 4:23 PM

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.

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?

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.

This revision was automatically updated to reflect the committed changes.

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 :-(