servo / saltfs

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(DO NOT MERGE) Refactor builds to allow non-Servo builds on buildbot. #721

Closed

metajack wants to merge 1 commit into servo:master from metajack:giturl-builds

Contributor

metajack commented Sep 26, 2017 •

edited by larsbergstrom

This change is

highfive assigned aneeshusa

highfive added the S-awaiting-review label

Contributor Author

metajack commented Sep 26, 2017


        Refactor builds to allow non-Servo builds on buildbot.

ceaf003

metajack force-pushed the metajack:giturl-builds branch from e9566d3 to ceaf003

aneeshusa suggested changes

Member

aneeshusa left a comment

Overall seems reasonable.
I'd recommend reviewing #722 first so we can keep line lengths down and avoid hitting the 80-char limit in the list of builders.

Also, StepsYAMLParsingStep currently has a bunch of Servo-specific behavior; from a quick glance I don't think it will get in the way of webrender at all, but it might be nice to split that out via subclass or something.

buildbot/master/files/config/factories.py

  
      @@ -67,8 +67,8 @@ def __init__(self, build_steps):
    
        class StepsYAMLParsingStep(buildstep.ShellMixin, buildstep.BuildStep):

            """\

            Step which reads the YAML steps configuration in the main servo repo

            and dynamically adds test steps.

            Step which reads the YAML steps configuration in the repo and dynamically

aneeshusa Sep 28, 2017

Member

the YAML steps -> a YAML steps
I'd also keep the second part of the sentence (and dynamically...) on the next line.

buildbot/master/files/config/master.cfg

  
            Builder which uses DynamicServoFactory to run steps

            from a YAML file in the main servo repo.

            Builder which uses DynamicGitUrlFactory to run steps

            from a YAML file in the repo.

aneeshusa Sep 28, 2017

Member

in the repo -> in a repo

buildbot/master/files/config/master.cfg

  
                # util.BuilderConfig is an old-style class so we cannot use super()

                # but must hardcode the superclass here

                util.BuilderConfig.__init__(

                    self,

                    name=name,

                    slavenames=slavenames,

                    factory=factories.DynamicServoFactory(name, environment),

                    factory=factories.DynamicGitUrlFactory("https://github.com/%s/%s" % (repo_owner, repo_name),

aneeshusa Sep 28, 2017

Member

I prefer block indent and .format.

buildbot/master/files/config/master.cfg

  
                    factory=factories.DynamicServoFactory(name, environment),

                    factory=factories.DynamicGitUrlFactory("https://github.com/%s/%s" % (repo_owner, repo_name),

                                                           name, environment,

                                                           "etc/ci/buildbot_steps.yml"),

aneeshusa Sep 28, 2017

Member

Should we hardcode this path? Webrender will need its own steps.yaml file, and it seems a little silly to add etc/ci directories just for that. I'm not opposed to leaving this hardcoded, though.

buildbot/master/files/config/master.cfg

  
                                envs.build_windows_msvc),

            DynamicServoBuilder("windows-msvc-nightly", WINDOWS_SLAVES,

                                envs.build_windows_msvc),

            DynamicRepoBuilder("servo", "servo", "android", CROSS_SLAVES, envs.build_android),

aneeshusa Sep 28, 2017

Member

It would be nice to keep DynamicServoBuilder as a subclass of DynamicRepoBuilder to cut down on a lot of this duplication, reduce diff noise, and also avoid hitting the 80-char line limit.

buildbot/master/files/config/master.cfg

  
                               envs.build_windows_msvc),

            DynamicRepoBuilder("servo", "servo", "windows-msvc-nightly", WINDOWS_SLAVES,

                               envs.build_windows_msvc),

            DyanmicRepoBuilder("servo", "webrender", "linux-repo", LINUX_SLAVES, envs.build_linux),

aneeshusa Sep 28, 2017

Member

How about webrender-linux or wr-linux? repo is a bit generic.
Same comment for the mac builder.

aneeshusa mentioned this pull request

Infer Buildbot workers from environment #722

Closed

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels

Contributor

bors-servo commented Jul 12, 2018

☔ The latest upstream changes (presumably #852) made this pull request unmergeable. Please resolve the merge conflicts.

highfive added the S-needs-rebase label

jdm closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.