[Mesa-dev] [PATCH 1/3] mesa: Add toplevel Android.mk

[Chia-I Wu]

On Wed, Aug 17, 2011 at 9:33 AM, Chad Versace <chad at chad-versace.us> wrote:
> (I accidentally hit the send button mid-sentence on the last email.
> Please ignore it).
>
> On Tue, Aug 16, 2011 at 16:38, Chia-I Wu <olv at lunarg.com> wrote:
>> On Wed, Aug 17, 2011 at 5:59 AM, Chad Versace <chad at chad-versace.us> wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 08/16/2011 12:30 AM, Chia-I Wu wrote:
>>>> Hi Chad,
>>>>
>>>> On Tue, Aug 16, 2011 at 2:14 PM, Chad Versace <chad at chad-versace.us>
>>>> wrote:
>>>>> This is the first step in porting libGLES* and libEGL to Android.
>>>> I also has a branch[1] that is ready for merge IMHO and is known to work.
>>>> I do not want to run into a hypothetical situation that we review and NAK
>>>> each other.  Shouldn't it be better if we and the list discuss what is the
>>>> best way to merge Android support before going on[2]?
>>>
>>> I also wish to avoid a mutual NAK quagmire. Perhaps if we each explicitly list
>>> our immediate and longterm goals, we can find common ground and move forward.
>>>
>>> Immediate Goals
>>> - ---------------
>>> 1. That the i965 driver have alpha quality sometime in October, though sooner
>>> is better.
>>> 2. That the Android build rarely break for Intel. (This necessitates factoring
>>> out common source lists into sources.mk's).
>>> 3. That Android support exist on master.
>>>
>>> Longterm Goals
>>> - --------------
>>> 4. That the Android i965 driver be well-tested with Piglit.
>>>
>>> Regarding goal 2, I think your branch needs reworking.
>>>
>>>> I will talk about my approach first.  I plan to submit a series of patches
>>>> that are either small or isolated.  They can be reviewed as usual.  Then
>>>> there is a patch that adds Android.mk's to mesa's source tree.  The patch
>>>> only adds new files for use by Android, and nothing else is touched.  The
>>>> idea is that at that point, the Android support is known to work, and no
>>>> regression is possible with the existing ways of building mesa.  Those
>>>> familiar with Android's build system can review the new build system as a
>>>> whole by just looking at this patch and give it a try, instead of looking
>>>> at a fraction at a time.
>>>>
>>>> The upside of this approach is that it works and is ready for review. The
>>>> downside is that it suffers from the same pain as SCons does: when Makefile
>>>> is changed, Android.mk needs to be updated.
>>>>
>>>> If that is a concern,
>>>
>>> That is is serious concern of mine. I do not want the i965 driver and
>>> associated libraries to break when their respective Makefiles change.
>>>
>>>> then I would like to suggest another approach that needs help from you
>>>> and/or José.  The idea is for us to isolate the bits in Makefile's that can
>>>> be shared with SCons and move them to, say, sources.mk's.  Then we can
>>>> include sources.mk's in Makefile's and have SCons parses them too.  This
>>>> way we can improve the existing build systems without cluttering mesa.
>>>> Then, I will redo my last patch to use sources.mk's.
>>>>
>>>> Do you have other concerns regarding either approach?
>>>
>>> If we take the approach of "commit all the working Android.mk's now, then
>>> cleanup later", then all the Android.mk's will immediately require fixing in
>>> order to use the sources.mk's. This effectively makes those Android.mk's
>>> throw-away code.
>>>
>>> I insist that we do not commit throw-away code. At best, throw-away code
>>> eventually does get fixed, but at the cost of laborious code-churn. At worst,
>>> it languishes in disrepair due to procrastination.
>> These concerns all seem to suggest that we should factor out common
>> source list to sources.mk's first.  I think we can agree on this and
>> continue from here.
> Agreed. I will begin factoring out the source.mk's tomorrow.
Great.  I will see how it goes and look into how to make SCons pick them up.

>> I prefer not to have such clean-up work disguised as Android support.
>> Firstly, for people reviewing the clean-up work, changes for Android
>> are noises, and vice versa.  And since the patches may be sent out
>> over days, there is no guarantee that we won't end up with a partially
>> converted tree.  I do not suggest that everything should be converted.
>>  But it makes sense to convert those that can benefit SCons.
>
> It also makes sense to convert those that can benefit Android.
Yes.
> For the Intel directories, it matters not to me if the cleanup patches are
> interleaved with "real Android" patches. But outside of the Intel dirs, I'll
> follow your suggestion and post cleanup patches separately.
>
>>>> As implied by above, the main concern I have with your approach (from the
>>>> last series) is that I could not verify it and see the whole thing come
>>>> together.  You could end up doing what I have been doing for Android-x86,
>>>> and I may have to redo it again for gallium provided your approach is
>>>> adopted.  The other concern is that I do not think
>>>>
>>>> patch 1: fix src/mesa/Makefile patch 2: add src/mesa/Android.mk patch 3:
>>>> fix src/mesa/drivers/Makefile patch 4: add src/mesa/drivers/Android.mk
>>>> (follow this pattern for each directory)
>>>>
>>>> is cleaner than fixing Makefile's first (say for SCons) and then add
>>>> Android.mk's as a third build system.
>>>
>>> I do not think posting, or committing, a 2382 line patch is a good idea
>>> either. Such a patch is impossible to meaningfully review. No one can
>>> understand it in its entirety, and no one can post inline comments.
>> I can split the patch up into as many patches as they make sense
>> (within a day, if that helps).
>
> You have many patches on your tree, such as the EGL patches, that are
> good to go *today*. I see no reason to hesitate any longer with your
> non-makefile patches, and I would like to see them reviewed and
> committed.
>
>> Also, my concerns regarding your series are not addressed.  It is hard
>> to verify the work (e.g., it could run into issues when linking the
>> final i965_dri).
>
> The series is only 3 patches, and the makefiles I posted are simple
> and short and do exactly what the commit messages say. Hence any
> problem that the series contains should be easy to identify. If you
> have identified a problem with the way I built libglapi, then state
> the problem.
I've commented on one of the patches.
>> Suppose everything builds, do you plan to integrate
>> with Android or Android-x86 and how?
>
> This series *is* integrated with Android-x86, as you can `git am`
> these patches onto master and android-x86/external/mesa will
> successfully build. Eventually, when Android support is complete,
> there (hopefully) will be no Android-x86 integration issues. because
> Android-x86 will (hopefully) just use Mesa master.
>
> If you have a concrete integration concern, then please elaborate.
It seems libglapi will be built unconditionally, even on targets that
do not use mesa.  This is all I've got as this series is small.
> I also have future plans to integrate the build into Intel's Android tree.
While your patches are easy to read, cleanly formatted, I do not feel
like like we are working in an FOSS fashion sometimes.  Our works
overlap.  We might be the ones to review each other, and we are both
qualified enough to NAK each other given good reasons.  I don't think
keep pushing (subset of) changes for review is better than resolving
the conflicts between us first.  We made progress agreeing that the
common source list should be factored out, even this series has
nothing do with that, don't we?  I knew that you prefer one patch for
each Android.mk, and I agreed to do that, right?

But perhaps it is just me who do not think code talks better here.  I
will roll out my series and see how it goes.

> --
> Chad Versace
> chad at chad-versace.us
>
>> There could be more questions,
>> to both our works.  I think this is another reason not to mix
>> factoring out source lists to sources.mk's with Android support.
>>>
>>> - --
>>> Chad Versace
>>> chad at chad-versace.us
>>
>>
>>
>> --
>> olv at LunarG.com
>>
>



-- 
olv at LunarG.com