diff mbox

autobuild-run: add --repo option

Message ID 20170717212859.12581-1-arnout@mind.be
State Accepted
Headers show

Commit Message

Arnout Vandecappelle July 17, 2017, 9:28 p.m. UTC
This option allows to specify which Buildroot git repository to clone.
This can be useful in several situations:
- to use a different mirror in case you don't have a good connection to
  github;
- for debugging this script;
- to point to a local, patched repository you want to test.

Note that the clone/pull will use the currently checked out branch of
the repository if it is non-bare, which is ideal for testing.

Since switching repositories may also switch branches, we use a git
fetch/checkout sequence instead of doing a git pull. With git pull, the
branches would be merged instead of switched. To avoid polluting the
log with the long git message about a detached head, while still
getting some useful output from git, pass the --detach option to
git checkout.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
Ideally the contents of this commit message would be put somewhere as
documentation, but I couldn't find a good spot for it.
---
 scripts/autobuild-run | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Matt Weber July 18, 2017, 2:45 a.m. UTC | #1
Arnout,

On Mon, Jul 17, 2017 at 4:28 PM, Arnout Vandecappelle (Essensium/Mind)
<arnout@mind.be> wrote:
> This option allows to specify which Buildroot git repository to clone.
> This can be useful in several situations:
> - to use a different mirror in case you don't have a good connection to
>   github;
> - for debugging this script;
> - to point to a local, patched repository you want to test.
>
> Note that the clone/pull will use the currently checked out branch of
> the repository if it is non-bare, which is ideal for testing.
>
> Since switching repositories may also switch branches, we use a git
> fetch/checkout sequence instead of doing a git pull. With git pull, the
> branches would be merged instead of switched. To avoid polluting the
> log with the long git message about a detached head, while still
> getting some useful output from git, pass the --detach option to
> git checkout.
>
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

I carry some local changes for tailoring the repo for similar reasons
(I while back I submitted something similar before the input parsing
was cleaned up).  What do you think about also making it a config file
option?

Reviewed-by: Matt Weber <matthew.weber@rockwellcollins.com>

> ---
> Ideally the contents of this commit message would be put somewhere as
> documentation, but I couldn't find a good spot for it.
> ---
>  scripts/autobuild-run | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/autobuild-run b/scripts/autobuild-run
> index a7d7d4f..523e382 100755
> --- a/scripts/autobuild-run
> +++ b/scripts/autobuild-run
> @@ -68,6 +68,7 @@ defaults = {
>      '--pid-file': '/tmp/buildroot-autobuild.pid',
>      '--http-url': 'http://autobuild.buildroot.org/submit/',
>      '--toolchains-url': 'http://autobuild.buildroot.org/toolchains/configs/toolchain-configs.csv',
> +    '--repo': 'https://github.com/buildroot/buildroot.git',
>  }
>
>  doc = """autobuild-run - run Buildroot autobuilder
> @@ -103,6 +104,8 @@ Options:
>                                   Not set by default.
>    -d, --debug                    Send log output to stdout instead of log file
>    --toolchains-url URL           URL of toolchain configuration file
> +  -r, --repo URL                 URL of Buildroot repository to clone
> +                                 Defaults to %(--repo)s
>
>  Format of the configuration file:
>
> @@ -329,7 +332,7 @@ def prepare_build(**kwargs):
>      # didn't exist already.
>      srcdir = os.path.join(idir, "buildroot")
>      if not os.path.exists(srcdir):
> -        ret = subprocess.call(["git", "clone", "https://github.com/buildroot/buildroot.git", srcdir],
> +        ret = subprocess.call(["git", "clone", kwargs['repo'], srcdir],
>                                stdout=log, stderr=log)
>          if ret != 0:
>              log_write(log, "ERROR: could not clone Buildroot sources")
> @@ -337,9 +340,14 @@ def prepare_build(**kwargs):
>
>      # Update the Buildroot sources.
>      abssrcdir = os.path.abspath(srcdir)
> -    ret = subprocess.call(["git", "pull"], cwd=abssrcdir, stdout=log, stderr=log)
> +    ret = subprocess.call(["git", "fetch", kwargs['repo']], cwd=abssrcdir, stdout=log, stderr=log)
>      if ret != 0:
> -        log_write(log, "ERROR: could not pull Buildroot sources")
> +        log_write(log, "ERROR: could not fetch Buildroot sources")
> +        return -1
> +
> +    ret = subprocess.call(["git", "checkout", "--detach", "FETCH_HEAD"], cwd=abssrcdir, stdout=log, stderr=log)
> +    if ret != 0:
> +        log_write(log, "ERROR: could not check out Buildroot sources")
>          return -1
>
>      # Create an empty output directory. We remove it first, in case a previous build was aborted.
> @@ -948,6 +956,7 @@ def main():
>                  make_opts = (args['--make-opts'] or ''),
>                  nice = (args['--nice'] or 0),
>                  toolchains_url = args['--toolchains-url'],
> +                repo = args['--repo'],
>                  upload = upload,
>                  buildpid = buildpid,
>                  debug = args['--debug']
> --
> 2.13.2
>
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Arnout Vandecappelle July 18, 2017, 7:24 a.m. UTC | #2
On 18-07-17 04:45, Matthew Weber wrote:
> Arnout,
> 
> On Mon, Jul 17, 2017 at 4:28 PM, Arnout Vandecappelle (Essensium/Mind)
> <arnout@mind.be> wrote:
>> This option allows to specify which Buildroot git repository to clone.
>> This can be useful in several situations:
>> - to use a different mirror in case you don't have a good connection to
>>   github;
>> - for debugging this script;
>> - to point to a local, patched repository you want to test.
>>
>> Note that the clone/pull will use the currently checked out branch of
>> the repository if it is non-bare, which is ideal for testing.
>>
>> Since switching repositories may also switch branches, we use a git
>> fetch/checkout sequence instead of doing a git pull. With git pull, the
>> branches would be merged instead of switched. To avoid polluting the
>> log with the long git message about a detached head, while still
>> getting some useful output from git, pass the --detach option to
>> git checkout.
>>
>> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> I carry some local changes for tailoring the repo for similar reasons
> (I while back I submitted something similar before the input parsing
> was cleaned up).  What do you think about also making it a config file
> option?

 Sounds good to me. Feel free to do that and resubmit together with this one as
a series, or in a single patch.

 Regards,
 Arnout
Thomas Petazzoni July 18, 2017, 7:57 a.m. UTC | #3
Hello,

On Mon, 17 Jul 2017 23:28:59 +0200, Arnout Vandecappelle
(Essensium/Mind) wrote:
> This option allows to specify which Buildroot git repository to clone.
> This can be useful in several situations:
> - to use a different mirror in case you don't have a good connection to
>   github;
> - for debugging this script;
> - to point to a local, patched repository you want to test.
> 
> Note that the clone/pull will use the currently checked out branch of
> the repository if it is non-bare, which is ideal for testing.
> 
> Since switching repositories may also switch branches, we use a git
> fetch/checkout sequence instead of doing a git pull. With git pull, the
> branches would be merged instead of switched. To avoid polluting the
> log with the long git message about a detached head, while still
> getting some useful output from git, pass the --detach option to
> git checkout.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

My concern about such change (which I've already expressed in the past)
is to see build results submitted to the public autobuild.b.o instance
that do not correspond to an upstream Buildroot version.

Would it be reasonable to request that if a custom repo is used, then
uploading the results to autobuild.b.o is not possible ?

Or am I too zealous about this ?

Thomas
Arnout Vandecappelle July 18, 2017, 8:12 a.m. UTC | #4
On 18-07-17 09:57, Thomas Petazzoni wrote:
> Hello,
> 
> On Mon, 17 Jul 2017 23:28:59 +0200, Arnout Vandecappelle
> (Essensium/Mind) wrote:
>> This option allows to specify which Buildroot git repository to clone.
>> This can be useful in several situations:
>> - to use a different mirror in case you don't have a good connection to
>>   github;
>> - for debugging this script;
>> - to point to a local, patched repository you want to test.
>>
>> Note that the clone/pull will use the currently checked out branch of
>> the repository if it is non-bare, which is ideal for testing.
>>
>> Since switching repositories may also switch branches, we use a git
>> fetch/checkout sequence instead of doing a git pull. With git pull, the
>> branches would be merged instead of switched. To avoid polluting the
>> log with the long git message about a detached head, while still
>> getting some useful output from git, pass the --detach option to
>> git checkout.
>>
>> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> 
> My concern about such change (which I've already expressed in the past)
> is to see build results submitted to the public autobuild.b.o instance
> that do not correspond to an upstream Buildroot version.
> 
> Would it be reasonable to request that if a custom repo is used, then
> uploading the results to autobuild.b.o is not possible ?

 But Matt's use case is exactly that he doesn't have a good connection to the
upstream repo so he wants to use a mirror, but still upload to a.b.o.

 Admittedly, this is exactly what lead to the recent problems with Matt's
autobuilder...


> Or am I too zealous about this ?

 Yes you are. Without this change, someone like Matt will just edit the
autobuild-run script and you still have the same problem :-)

 Perhaps we can make the problem at least detectable, by pushing *kwargs to
a.b.o in addition to the BR git ID. But that obviously requires modifications on
a.b.o itself, which is a bit more difficult to organize I guess.


 Since you're the BDFL for autobuild-run, feel free to follow up this patch with
something that disables uploading if --repo is non-default. And let Matt solve
his own problems :-)

 Regards,
 Arnout
Thomas Petazzoni July 18, 2017, 8:19 a.m. UTC | #5
Hello,

On Tue, 18 Jul 2017 10:12:11 +0200, Arnout Vandecappelle wrote:

> > Would it be reasonable to request that if a custom repo is used, then
> > uploading the results to autobuild.b.o is not possible ?  
> 
>  But Matt's use case is exactly that he doesn't have a good connection to the
> upstream repo so he wants to use a mirror, but still upload to a.b.o.
> 
>  Admittedly, this is exactly what lead to the recent problems with Matt's
> autobuilder...

Exactly my point: Matt's experience has shown that using a non-default
repository was causing problems, hence my resistance to this change.

In addition, if one is not able to fetch from https://github.com in a
reliable way, I don't see how one can decently run an autobuilder
instance that will anyway require download tons of stuff from various
places on the Internet.

> > Or am I too zealous about this ?  
> 
>  Yes you are. Without this change, someone like Matt will just edit the
> autobuild-run script and you still have the same problem :-)
> 
>  Perhaps we can make the problem at least detectable, by pushing *kwargs to
> a.b.o in addition to the BR git ID. But that obviously requires modifications on
> a.b.o itself, which is a bit more difficult to organize I guess.
> 
>  Since you're the BDFL for autobuild-run, feel free to follow up this patch with
> something that disables uploading if --repo is non-default. And let Matt solve
> his own problems :-)

Or I could add a server-side check that the git commit being reported
exists in the upstream Buildroot, and is not more than 1 or 2 days old.
It probably makes more sense to do this server-side. Which means I
could merge your --repo option (even if I still find its use cases
very, very dubious).

Best regards,

Thomas
Arnout Vandecappelle July 18, 2017, 8:25 a.m. UTC | #6
On 18-07-17 10:19, Thomas Petazzoni wrote:
[snip]
> Or I could add a server-side check that the git commit being reported
> exists in the upstream Buildroot, and is not more than 1 or 2 days old.
> It probably makes more sense to do this server-side.

 Probably yes, because indeed, something like Matt's problem could occur for
other reasons as well.

> Which means I could merge your --repo option (even if I still find its use cases
> very, very dubious).

 Well, my own use case was to be able to test the autobuild-run script with the
in-tree config generator. When that happens, it does make a lot of sense to be
able to test any change to the config generator with autobuild-run. Currently
you can test the change by just running the modified autobuild-run, but when the
config generator comes from the Buildroot tree that's not possible anymore.

 I was going to post all those changes together, but since Matt was mentioning
he had local changes to use a different mirror, I thought I'd post this one
individually already.

 Regards,
 Arnout
Matt Weber July 18, 2017, 11:46 a.m. UTC | #7
All,

On Tue, Jul 18, 2017 at 3:25 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
> On 18-07-17 10:19, Thomas Petazzoni wrote:
> [snip]
>> Or I could add a server-side check that the git commit being reported
>> exists in the upstream Buildroot, and is not more than 1 or 2 days old.
>> It probably makes more sense to do this server-side.
>
>  Probably yes, because indeed, something like Matt's problem could occur for
> other reasons as well.
>
>> Which means I could merge your --repo option (even if I still find its use cases
>> very, very dubious).
>
>  Well, my own use case was to be able to test the autobuild-run script with the
> in-tree config generator. When that happens, it does make a lot of sense to be
> able to test any change to the config generator with autobuild-run. Currently
> you can test the change by just running the modified autobuild-run, but when the
> config generator comes from the Buildroot tree that's not possible anymore.
>
>  I was going to post all those changes together, but since Matt was mentioning
> he had local changes to use a different mirror, I thought I'd post this one
> individually already.
>

I'm fine carrying a local change for a different repo/branch so that I
can do offline testing or use a mirror if needed for upstream.
However, your comments are fair about safe guarding results server
side.

Matt
Thomas Petazzoni July 25, 2017, 8:24 p.m. UTC | #8
Hello,

On Mon, 17 Jul 2017 23:28:59 +0200, Arnout Vandecappelle
(Essensium/Mind) wrote:
> This option allows to specify which Buildroot git repository to clone.
> This can be useful in several situations:
> - to use a different mirror in case you don't have a good connection to
>   github;
> - for debugging this script;
> - to point to a local, patched repository you want to test.
> 
> Note that the clone/pull will use the currently checked out branch of
> the repository if it is non-bare, which is ideal for testing.
> 
> Since switching repositories may also switch branches, we use a git
> fetch/checkout sequence instead of doing a git pull. With git pull, the
> branches would be merged instead of switched. To avoid polluting the
> log with the long git message about a detached head, while still
> getting some useful output from git, pass the --detach option to
> git checkout.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
> Ideally the contents of this commit message would be put somewhere as
> documentation, but I couldn't find a good spot for it.
> ---
>  scripts/autobuild-run | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Applied to buildroot-test, thanks.

Thomas
diff mbox

Patch

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index a7d7d4f..523e382 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -68,6 +68,7 @@  defaults = {
     '--pid-file': '/tmp/buildroot-autobuild.pid',
     '--http-url': 'http://autobuild.buildroot.org/submit/',
     '--toolchains-url': 'http://autobuild.buildroot.org/toolchains/configs/toolchain-configs.csv',
+    '--repo': 'https://github.com/buildroot/buildroot.git',
 }
 
 doc = """autobuild-run - run Buildroot autobuilder
@@ -103,6 +104,8 @@  Options:
                                  Not set by default.
   -d, --debug                    Send log output to stdout instead of log file
   --toolchains-url URL           URL of toolchain configuration file
+  -r, --repo URL                 URL of Buildroot repository to clone
+                                 Defaults to %(--repo)s
 
 Format of the configuration file:
 
@@ -329,7 +332,7 @@  def prepare_build(**kwargs):
     # didn't exist already.
     srcdir = os.path.join(idir, "buildroot")
     if not os.path.exists(srcdir):
-        ret = subprocess.call(["git", "clone", "https://github.com/buildroot/buildroot.git", srcdir],
+        ret = subprocess.call(["git", "clone", kwargs['repo'], srcdir],
                               stdout=log, stderr=log)
         if ret != 0:
             log_write(log, "ERROR: could not clone Buildroot sources")
@@ -337,9 +340,14 @@  def prepare_build(**kwargs):
 
     # Update the Buildroot sources.
     abssrcdir = os.path.abspath(srcdir)
-    ret = subprocess.call(["git", "pull"], cwd=abssrcdir, stdout=log, stderr=log)
+    ret = subprocess.call(["git", "fetch", kwargs['repo']], cwd=abssrcdir, stdout=log, stderr=log)
     if ret != 0:
-        log_write(log, "ERROR: could not pull Buildroot sources")
+        log_write(log, "ERROR: could not fetch Buildroot sources")
+        return -1
+
+    ret = subprocess.call(["git", "checkout", "--detach", "FETCH_HEAD"], cwd=abssrcdir, stdout=log, stderr=log)
+    if ret != 0:
+        log_write(log, "ERROR: could not check out Buildroot sources")
         return -1
 
     # Create an empty output directory. We remove it first, in case a previous build was aborted.
@@ -948,6 +956,7 @@  def main():
                 make_opts = (args['--make-opts'] or ''),
                 nice = (args['--nice'] or 0),
                 toolchains_url = args['--toolchains-url'],
+                repo = args['--repo'],
                 upload = upload,
                 buildpid = buildpid,
                 debug = args['--debug']