Page MenuHomePhabricator

Introduce Fuchsia builder
ClosedPublic

Authored by phosek on Dec 20 2018, 1:59 AM.

Details

Summary

This change introduces Fuchsia builder. This builder compiles a
complete Fuchsia toolchain in Linux host, using a two-stage build.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Dec 20 2018, 1:59 AM
gkistanova requested changes to this revision.Wed, Jan 2, 12:25 PM

Hello Petr,

Thanks for working on this!

On what commits do you want this builder to do a build?
As it is currently coded it would never work.

Please also see the in-lined notes.

zorg/buildbot/builders/FuchsiaBuilder.py
19 ↗(On Diff #179027)

Would you not use mutable default arguments in function definitions,please? This has quite bad side effects which are hard to debug. You can find more details at https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html.

20 ↗(On Diff #179027)

The same is here.

36 ↗(On Diff #179027)

You do not need the get_builddir property, really. The current build directory is always available as a build property and could be accessed as %(workdir)s everywhere below instead of %(builddir)s .

83 ↗(On Diff #179027)

Could you also support the clean and clean_obj build properties, please? To have this unified with other builders.

86 ↗(On Diff #179027)

Could you flip the condition over and check it for is None, instead, please? This is a cosmetic, and would help reading the code.

This revision now requires changes to proceed.Wed, Jan 2, 12:25 PM
phosek updated this revision to Diff 179988.Wed, Jan 2, 5:58 PM
phosek marked 5 inline comments as done.
phosek added a comment.Wed, Jan 2, 6:00 PM

On what commits do you want this builder to do a build?
As it is currently coded it would never work.

Thanks for the review! I noticed the missing scheduler after you pointed it out and I've added a new scheduler for this builder.

Do you know if there is there a way to test the builder factory manually?

It is better to use the zorg.buildbot.process.factory.LLVMBuildFactory instead of buildbot.process.factory.BuildFactory(). Zorg would assign it to the correct scheduler automatically in this case.

Anyway, there is still a few problems with scheduling this builder.

The scheduler listens for commits in SVN as the only point if truth. But you are checking out the source code from github. There is a racing condition, as the change might not yet be ready in the github repo. And revision number provided by the scheduler would not work for github as expected either.

May I suggest checking out from SVN till we would officially move to github?

phosek added a comment.Thu, Jan 3, 6:21 PM

It is better to use the zorg.buildbot.process.factory.LLVMBuildFactory instead of buildbot.process.factory.BuildFactory(). Zorg would assign it to the correct scheduler automatically in this case.

I'm fine switching to LLVMBuildFactory to make it more unified with other builders.

Anyway, there is still a few problems with scheduling this builder.

The scheduler listens for commits in SVN as the only point if truth. But you are checking out the source code from github. There is a racing condition, as the change might not yet be ready in the github repo. And revision number provided by the scheduler would not work for github as expected either.

May I suggest checking out from SVN till we would officially move to github?

I could do that, but our builder already assumes the flat (monorepo) layout so I'd need to implement the SVN checkout separately rather than relying on LLVMBuildFactory which uses the hierarchical layout.

During the GitHub migration panel at the last Developers' Meeting, it was suggested that after the new repository is available which according to last email on the thread should be some time next week, bot maintainers should start migrating away from SVN to Git. Do you have any estimate for when the LLVMPoller will be ready to support the new Git repository as well? I'd be fine waiting a few more days and start immediately on Git, but if it's going to take longer, I'd be fine starting with SVN and then switch to Git later.

phosek updated this revision to Diff 180188.Thu, Jan 3, 6:38 PM
serge-sans-paille resigned from this revision.Fri, Jan 4, 1:19 AM

Thanks,

Could you clean off the unused imports, please?
You could test how the factory works on the silent master. I'll help if any debugging would be needed.

phosek updated this revision to Diff 180610.Mon, Jan 7, 9:48 PM

Could you clean off the unused imports, please?

Done

gkistanova accepted this revision.Tue, Jan 8, 4:43 PM

Thanks, Petr!
LGTM

This revision is now accepted and ready to land.Tue, Jan 8, 4:43 PM
Closed by commit rL351724: Fuchsia builder (authored by phosek, committed by ). · Explain WhySun, Jan 20, 9:58 PM
This revision was automatically updated to reflect the committed changes.