You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by suraj acharya <su...@gmail.com> on 2016/09/13 21:57:02 UTC
Re: Pre-commit Check Avro
Hi,
When I reached out to the Apache Yetus dev mailing list for some
information regarding the addition of precommit check.
I got the reply from them which said that AVRO might want to change the
approach of which they run the tests.
I wanted to know if everyone was okay with what was being suggested before
I go around trying to make it work.
What is the opinion of people ?
---------- Forwarded message ----------
> From: Allen Wittenauer <aw...@effectivemachines.com>
> Date: Tue, Sep 13, 2016 at 8:13 AM
> Subject: Re: Avro Pre-commit.
> To: dev@yetus.apache.org
>
>
> I've been thinking about this issue. With a bit of work, I think the
> approach should probably be to:
> a) make a custom build tool that knows which dirs use which other
> tools
> b) add a new file in each dir that can tell precommit where to
> break for modules
>
> For example, let's say we have a dir structure like this:
>
> build.sh
> c/
> Makefile
> java/
> pom.xml
> perl/
> Makefile.PL
>
> Let's add a file called multibuild.sh that precommit can use:
>
> build.sh
> multibuild.sh
> c/
> Makefile
> multibuild.sh
> java/
> pom.xml
> multibuild.sh
> perl/
> Makefile.PL
> multibuild.sh
>
>
> In our "multibuild" build tool plugin, we'd define
>
> function multibuild_buildfile
> {
> echo "multibuild.sh"
> }
>
> Now when precommit gets a patch, it will know that a change in c/
> is a different module than a change in java/ or perl/ because of the
> presence of the multibuild.sh file. This prevents the "build the world"
> problem for a largely diverse project like avro.
>
> The place where this gets hard is our theoretical
> multibuild_executor function because precommit doesn't pass as a param the
> module that's currently being processed. (we should probably fix that) But
> we can still make it work and reality, it might have led us down a false
> path. If we make this function look like this:
>
> function multibuild_executor
> {
> echo "multibuild.sh"
> }
>
> and make sure that BUILDTOOLCWD=module (the default), then precommit will
> run our file that we placed to actually do the work. This file can then
> take whatever parameters we're getting passed and convert it as appropriate
> to the build tool for that directory. Since this is a custom build tool,
> you can dictate what parameters to pass to which modules via
> _modules_workers. For example, an easy route might be:
>
> function multibuild_modules_workers
> {
> modules_workers $2
> }
>
> then multibuild.sh for each language becomes a series of case statements.
> For C, it might look like:
>
> testname=$1
> shift
>
> case ${testname} in
> compile)
> make -Dcustomflag=1
> ;;
> doc)
> make man
> unit)
> make -Danotherflag=1 test
> ;;
> *)
> ;;
> esac
>
> It's pretty hacky, but theoretically it would work until we get a
> chance to make precommit smarter about multiple build tools in a single
> project. (which would be a pretty hard undertaking, but something we should
> probably do)
>
>
> > On Sep 10, 2016, at 6:32 PM, suraj acharya <su...@gmail.com> wrote:
> >
> > Yes.
> > The ./build.sh test
> > <https://github.com/apache/avro/blob/master/build.sh#L40> command helps
> run
> > all the tests from the top level.
> > However, I am not sure if we need to run all the tests when a change is
> > made?
> > I was thinking of running language specific tests for changes in the
> > specific directory.
> > For example for changes in python will trigger ant test
> > <https://github.com/apache/avro/blob/master/build.sh#L43>.
>
-Suraj Acharya
On Fri, Aug 19, 2016 at 9:54 AM, Sean Busbey <bu...@cloudera.com> wrote:
> If we have files that fail to include the required ASF licensing
> headers, you're correct that's important to fix.
>
> I think either working on it before or after would be fine. Ideally,
> Yetus Precommit should properly let us know when a patch is fixing
> those kinds of problems, so it would be a good validation that things
> are working.
>
> On Thu, Aug 18, 2016 at 8:18 PM, suraj acharya <su...@gmail.com>
> wrote:
> > Okay. I will look at the docker command and the image.
> >
> > One other question I have is that the asflicense is an important aspect
> of
> > yetus. However, many java files have that missing. And whenever a patch
> > touches that file it returns a -1. Do you think I should first fix all
> the
> > licenses and then continue with this?
> >
> > S
> >
> > -Suraj Acharya
> >
> > On Thu, Aug 18, 2016 at 12:37 PM, Sean Busbey <bu...@cloudera.com>
> wrote:
> >
> >> is the 3-5 minutes doing the tests across all of the language
> >> libraries or just the java ones?
> >>
> >> docker will definitely be needed, due to the number of different
> >> system dependencies needed to build the different languages.
> >> I was hoping we could reuse the Docker image that is currently
> >> used for the "./build.sh docker" command.
> >>
> >>
> >> On Wed, Aug 17, 2016 at 6:41 PM, suraj acharya <su...@gmail.com>
> >> wrote:
> >> > Hi,
> >> > I am a new guy and I have been working on setting up pre-commit check
> for
> >> > Avro : AVRO-1887 <https://issues.apache.org/jira/browse/AVRO-1887>.
> >> >
> >> > I was looking at the unit tests being run currently. It takes
> somewhere
> >> > around 3-5 minutes depending on the network and the repo's present.
> >> > I was thinking that since the time is so low why not run all the tests
> >> for
> >> > every commit. Also, the tests being lightweight I was thinking that
> there
> >> > is no need for docker to be needed.
> >> >
> >> > I would like to gather your opinions on this topic before going
> further
> >> > down this path.
> >> >
> >> > Thanks
> >> >
> >> > -Suraj Acharya
> >>
> >>
> >>
> >> --
> >> busbey
> >>
>
>
>
> --
> busbey
>
Re: Pre-commit Check Avro
Posted by Sean Busbey <bu...@cloudera.com>.
We have something like this now with our build.sh files, I think?
Busbey-MBA:avro busbey$ find . -name build.sh
./build.sh
./lang/c/build.sh
./lang/c++/build.sh
./lang/csharp/build.sh
./lang/js/build.sh
./lang/php/build.sh
./lang/ruby/build.sh
how about we just formalize this for the places where it's missing?
Then we can just map between the targets we recognize (test, dist,
etc) and the names yetus uses.
On Tue, Sep 13, 2016 at 2:57 PM, suraj acharya <su...@gmail.com> wrote:
> Hi,
> When I reached out to the Apache Yetus dev mailing list for some
> information regarding the addition of precommit check.
> I got the reply from them which said that AVRO might want to change the
> approach of which they run the tests.
> I wanted to know if everyone was okay with what was being suggested before
> I go around trying to make it work.
>
> What is the opinion of people ?
>
> ---------- Forwarded message ----------
>> From: Allen Wittenauer <aw...@effectivemachines.com>
>> Date: Tue, Sep 13, 2016 at 8:13 AM
>> Subject: Re: Avro Pre-commit.
>> To: dev@yetus.apache.org
>>
>>
>> I've been thinking about this issue. With a bit of work, I think the
>> approach should probably be to:
>
>
>> a) make a custom build tool that knows which dirs use which other
>> tools
>> b) add a new file in each dir that can tell precommit where to
>> break for modules
>>
>> For example, let's say we have a dir structure like this:
>>
>> build.sh
>> c/
>> Makefile
>> java/
>> pom.xml
>> perl/
>> Makefile.PL
>>
>> Let's add a file called multibuild.sh that precommit can use:
>>
>> build.sh
>> multibuild.sh
>> c/
>> Makefile
>> multibuild.sh
>> java/
>> pom.xml
>> multibuild.sh
>> perl/
>> Makefile.PL
>> multibuild.sh
>>
>>
>> In our "multibuild" build tool plugin, we'd define
>>
>> function multibuild_buildfile
>> {
>> echo "multibuild.sh"
>> }
>>
>> Now when precommit gets a patch, it will know that a change in c/
>> is a different module than a change in java/ or perl/ because of the
>> presence of the multibuild.sh file. This prevents the "build the world"
>> problem for a largely diverse project like avro.
>>
>> The place where this gets hard is our theoretical
>> multibuild_executor function because precommit doesn't pass as a param the
>> module that's currently being processed. (we should probably fix that) But
>> we can still make it work and reality, it might have led us down a false
>> path. If we make this function look like this:
>>
>> function multibuild_executor
>> {
>> echo "multibuild.sh"
>> }
>>
>> and make sure that BUILDTOOLCWD=module (the default), then precommit will
>> run our file that we placed to actually do the work. This file can then
>> take whatever parameters we're getting passed and convert it as appropriate
>> to the build tool for that directory. Since this is a custom build tool,
>> you can dictate what parameters to pass to which modules via
>> _modules_workers. For example, an easy route might be:
>>
>> function multibuild_modules_workers
>> {
>> modules_workers $2
>> }
>>
>> then multibuild.sh for each language becomes a series of case statements.
>> For C, it might look like:
>>
>> testname=$1
>> shift
>>
>> case ${testname} in
>> compile)
>> make -Dcustomflag=1
>> ;;
>> doc)
>> make man
>> unit)
>> make -Danotherflag=1 test
>> ;;
>> *)
>> ;;
>> esac
>>
>> It's pretty hacky, but theoretically it would work until we get a
>> chance to make precommit smarter about multiple build tools in a single
>> project. (which would be a pretty hard undertaking, but something we should
>> probably do)
>>
>>
>> > On Sep 10, 2016, at 6:32 PM, suraj acharya <su...@gmail.com> wrote:
>> >
>> > Yes.
>> > The ./build.sh test
>> > <https://github.com/apache/avro/blob/master/build.sh#L40> command helps
>> run
>> > all the tests from the top level.
>> > However, I am not sure if we need to run all the tests when a change is
>> > made?
>> > I was thinking of running language specific tests for changes in the
>> > specific directory.
>> > For example for changes in python will trigger ant test
>> > <https://github.com/apache/avro/blob/master/build.sh#L43>.
>>
>
> -Suraj Acharya
>
> On Fri, Aug 19, 2016 at 9:54 AM, Sean Busbey <bu...@cloudera.com> wrote:
>
>> If we have files that fail to include the required ASF licensing
>> headers, you're correct that's important to fix.
>>
>> I think either working on it before or after would be fine. Ideally,
>> Yetus Precommit should properly let us know when a patch is fixing
>> those kinds of problems, so it would be a good validation that things
>> are working.
>>
>> On Thu, Aug 18, 2016 at 8:18 PM, suraj acharya <su...@gmail.com>
>> wrote:
>> > Okay. I will look at the docker command and the image.
>> >
>> > One other question I have is that the asflicense is an important aspect
>> of
>> > yetus. However, many java files have that missing. And whenever a patch
>> > touches that file it returns a -1. Do you think I should first fix all
>> the
>> > licenses and then continue with this?
>> >
>> > S
>> >
>> > -Suraj Acharya
>> >
>> > On Thu, Aug 18, 2016 at 12:37 PM, Sean Busbey <bu...@cloudera.com>
>> wrote:
>> >
>> >> is the 3-5 minutes doing the tests across all of the language
>> >> libraries or just the java ones?
>> >>
>> >> docker will definitely be needed, due to the number of different
>> >> system dependencies needed to build the different languages.
>> >> I was hoping we could reuse the Docker image that is currently
>> >> used for the "./build.sh docker" command.
>> >>
>> >>
>> >> On Wed, Aug 17, 2016 at 6:41 PM, suraj acharya <su...@gmail.com>
>> >> wrote:
>> >> > Hi,
>> >> > I am a new guy and I have been working on setting up pre-commit check
>> for
>> >> > Avro : AVRO-1887 <https://issues.apache.org/jira/browse/AVRO-1887>.
>> >> >
>> >> > I was looking at the unit tests being run currently. It takes
>> somewhere
>> >> > around 3-5 minutes depending on the network and the repo's present.
>> >> > I was thinking that since the time is so low why not run all the tests
>> >> for
>> >> > every commit. Also, the tests being lightweight I was thinking that
>> there
>> >> > is no need for docker to be needed.
>> >> >
>> >> > I would like to gather your opinions on this topic before going
>> further
>> >> > down this path.
>> >> >
>> >> > Thanks
>> >> >
>> >> > -Suraj Acharya
>> >>
>> >>
>> >>
>> >> --
>> >> busbey
>> >>
>>
>>
>>
>> --
>> busbey
>>
--
busbey