Re: [edk2-devel] [PATCH 0/3] Arm builds on Visual Studio

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 3428 bytes --]



> On Sep 19, 2019, at 12:24 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 09/19/19 11:44, Leif Lindholm wrote:
>> Hi Liming,
>> 
>> On Thu, Sep 19, 2019 at 06:19:42AM +0000, Gao, Liming wrote:
>>> I add my comments. 
>>> 
>>>> -----Original Message-----
>>>> From: Baptiste Gerondeau [mailto:baptiste.gerondeau@linaro.org]
>>>> Sent: Thursday, September 19, 2019 12:05 AM
>>>> To: devel@edk2.groups.io
>>>> Cc: ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Zhang,
>>>> Shenglei <shenglei.zhang@intel.com>; Baptiste Gerondeau
>>>> <baptiste.gerondeau@linaro.org>
>>>> Subject: [PATCH 0/3] Arm builds on Visual Studio
>>>> 
>>>> EDIT: Resending the series since I mistakenly used the wrong email,
>>>> sorry !
>>>> 
>>>> We are currently making an effort to make ARM (and AARCH64 eventually)
>>>> builds using Microsoft's Visual Studio Compiler (aka MSVC/MSFT).
>>>> 
>>>> These 3 patches correspond to an effort to make the assembler work with
>>>> MSFT, which entails :
>>>> - Feeding MSFT the RVCT .asm files, since they share syntax
>>>> requirements.
>>> 
>>> Please separate the patch. Each patch is for each package, can't cross packages. 
>>> If so, the package maintainer can easy review the change. 
>> 
>> I agree with this as a general rule, but for this (hopefully never to
>> be repeated) operation, it makes sense to me to keep each change in
>> this set as one patch.
>> 
>> For the simple reason that the alternative leaves several unusable
>> commits in sequence in the repository. There is simply no way to
>> bisect through this change on a per-package basis.
>> 
>> This is after all a horrible horrible hack that lets us keep using the
>> .asm files provided for one toolchain family (RVCT) in a different
>> toolchain family (MSFT), without having to delete and re-add, losing
>> history in the process.
>> 
>> Would you be OK with an exception for this extremely unusual
>> situation?
> 
> (The question was posed to Liming, but I'm going to follow up here with
> my own thoughts, after getting CC'd by Liming. Thanks for that BTW.)
> 
> Let's assume the changes are split up with a fine granularity. Under
> that assumption, consider two cases:
> 
> (1) ARM builds on Visual Studio are broken until the last patch is
> applied, but all other toolchains continue working fine throughout the
> series.
> 
> versus
> 
> (2) ARM builds are broken on Visual Studio *and* on at least one other
> -- preexistent -- toolchain, until the last patch in the series is applied.
> 
> 
> Which case reflects reality?
> 
> If it's case (1), then I prefer the fine-grained patch series structure.
> Nothing regresses with existent toolchains (so bisection works with
> them), we just have to advertise the last patch in the series as the one
> that enables Visual Studio.
> 
> If it's case (2), then I agree with the larger (multi-package) patch, as
> a rare exception.
> 
> (We can also put it like this: if it's *possible* to write this series
> such that it enables (1), then we should strive for that.)
> 

Laszlo,

I agree with your logic around (1) and (2). I'd also point out we can do the review using (1) and squash into a single commit if we need to take path (2).

Thanks,

Andrew Fish

> Thanks,
> Laszlo
> 
> 


[-- Attachment #2: Type: text/html, Size: 25570 bytes --]