diff mbox series

[ovs-dev] datapath-windows: Specify external include paths

Message ID 20210615134308.236-1-aserdean@ovn.org
State Accepted
Headers show
Series [ovs-dev] datapath-windows: Specify external include paths | expand

Commit Message

Alin-Gabriel Serdean June 15, 2021, 1:43 p.m. UTC
VStudio 16.10 adds usermode includes before including the driver kit ones.

Bug tracked at:
https://developercommunity.visualstudio.com/t/error-lnk2019-unresolved-external-symbol-stdio-com/1434674

Fixes appveyor build reported by forcing external includes.

Reported-by: Ilya Maximets <i.maximets@ovn.org>
Reported-by: Frank Wagner <frank.wagner@dbosoft.eu>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/209#issuecomment-861385852
Reported-at: https://github.com/openvswitch/ovs-issues/issues/211
Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org>
---
 datapath-windows/ovsext/ovsext.vcxproj | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Ilya Maximets June 15, 2021, 4:06 p.m. UTC | #1
On 6/15/21 3:43 PM, Alin Gabriel Serdean wrote:
> VStudio 16.10 adds usermode includes before including the driver kit ones.
> 
> Bug tracked at:
> https://developercommunity.visualstudio.com/t/error-lnk2019-unresolved-external-symbol-stdio-com/1434674
> 
> Fixes appveyor build reported by forcing external includes.

Thanks, Alin.  I know nothing about the windows build process, but I see
that this patch fixes the issue with the current AppVeyor CI, therefore:

Acked-by: Ilya Maximets <i.maximets@ovn.org>

Out of curiosity, is this change backward compatible?  I mean,
is it possible to build on older platform (older VS) with this change?
Alin-Gabriel Serdean June 16, 2021, 3:06 p.m. UTC | #2
On Tue, 2021-06-15 at 18:06 +0200, Ilya Maximets wrote:
> On 6/15/21 3:43 PM, Alin Gabriel Serdean wrote:
> > VStudio 16.10 adds usermode includes before including the driver
> > kit ones.
> > 
> > Bug tracked at:
> > https://developercommunity.visualstudio.com/t/error-lnk2019-unresolved-external-symbol-stdio-com/1434674
> > 
> > Fixes appveyor build reported by forcing external includes.
> 
> Thanks, Alin.  I know nothing about the windows build process, but I
> see
> that this patch fixes the issue with the current AppVeyor CI,
> therefore:
> 
> Acked-by: Ilya Maximets <i.maximets@ovn.org>

Thank you!

> 
> Out of curiosity, is this change backward compatible?  I mean,
> is it possible to build on older platform (older VS) with this
> change?

It should be.
Usually we do not need to force the order of include directories. For
kernel projects it should default to the kernel includes.
I test with the last two versions of VS (2019, 2017).

We should add a build matrix for different versions of VS images to
appveyor / GHA so we could be sure.
I'll try to update the appveyor side.

FWIW a new version of VS was launched yesterday (
https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes#visual-studio-2019-version-1610-releases
), I will try to compile
without the patch to see if they hotfixed the issue.
Alin-Gabriel Serdean June 17, 2021, 9:31 a.m. UTC | #3
On Wed, 2021-06-16 at 18:06 +0300, Alin-Gabriel Serdean wrote:
> On Tue, 2021-06-15 at 18:06 +0200, Ilya Maximets wrote:
> > On 6/15/21 3:43 PM, Alin Gabriel Serdean wrote:
> > > VStudio 16.10 adds usermode includes before including the driver
> > > kit ones.
> > > 
> > > Bug tracked at:
> > > https://developercommunity.visualstudio.com/t/error-lnk2019-unresolved-external-symbol-stdio-com/1434674
> > > 
> > > Fixes appveyor build reported by forcing external includes.
> > 
> > Thanks, Alin.  I know nothing about the windows build process, but
> > I
> > see
> > that this patch fixes the issue with the current AppVeyor CI,
> > therefore:
> > 
> > Acked-by: Ilya Maximets <i.maximets@ovn.org>
> 
> Thank you!
> 
> > Out of curiosity, is this change backward compatible?  I mean,
> > is it possible to build on older platform (older VS) with this
> > change?
> 
> It should be.
> Usually we do not need to force the order of include directories. For
> kernel projects it should default to the kernel includes.
> I test with the last two versions of VS (2019, 2017).
> 
> We should add a build matrix for different versions of VS images to
> appveyor / GHA so we could be sure.
> I'll try to update the appveyor side.
> 
> FWIW a new version of VS was launched yesterday (
> https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes#visual-studio-2019-version-1610-releases
> ), I will try to compile
> without the patch to see if they hotfixed the issue.

It did not. Applying the patch.
Ilya Maximets June 17, 2021, 11:29 a.m. UTC | #4
On 6/17/21 11:31 AM, Alin-Gabriel Serdean wrote:
> On Wed, 2021-06-16 at 18:06 +0300, Alin-Gabriel Serdean wrote:
>> On Tue, 2021-06-15 at 18:06 +0200, Ilya Maximets wrote:
>>> On 6/15/21 3:43 PM, Alin Gabriel Serdean wrote:
>>>> VStudio 16.10 adds usermode includes before including the driver
>>>> kit ones.
>>>>
>>>> Bug tracked at:
>>>> https://developercommunity.visualstudio.com/t/error-lnk2019-unresolved-external-symbol-stdio-com/1434674
>>>>
>>>> Fixes appveyor build reported by forcing external includes.
>>>
>>> Thanks, Alin.  I know nothing about the windows build process, but
>>> I
>>> see
>>> that this patch fixes the issue with the current AppVeyor CI,
>>> therefore:
>>>
>>> Acked-by: Ilya Maximets <i.maximets@ovn.org>
>>
>> Thank you!
>>
>>> Out of curiosity, is this change backward compatible?  I mean,
>>> is it possible to build on older platform (older VS) with this
>>> change?
>>
>> It should be.
>> Usually we do not need to force the order of include directories. For
>> kernel projects it should default to the kernel includes.
>> I test with the last two versions of VS (2019, 2017).
>>
>> We should add a build matrix for different versions of VS images to
>> appveyor / GHA so we could be sure.
>> I'll try to update the appveyor side.
>>
>> FWIW a new version of VS was launched yesterday (
>> https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes#visual-studio-2019-version-1610-releases
>> ), I will try to compile
>> without the patch to see if they hotfixed the issue.
> 
> It did not. Applying the patch.
> 

OK.  CI is green now.  Thanks!

Best regards, Ilya Maximets.
Guru Shetty June 25, 2021, 10:14 p.m. UTC | #5
Alin,
 Does it make sense to apply this patch to stable branches too. I see that
2.15 fails with a similar error.

Thanks,
Guru

On Thu, 17 Jun 2021 at 02:31, Alin-Gabriel Serdean <aserdean@ovn.org> wrote:

> On Wed, 2021-06-16 at 18:06 +0300, Alin-Gabriel Serdean wrote:
> > On Tue, 2021-06-15 at 18:06 +0200, Ilya Maximets wrote:
> > > On 6/15/21 3:43 PM, Alin Gabriel Serdean wrote:
> > > > VStudio 16.10 adds usermode includes before including the driver
> > > > kit ones.
> > > >
> > > > Bug tracked at:
> > > >
> https://developercommunity.visualstudio.com/t/error-lnk2019-unresolved-external-symbol-stdio-com/1434674
> > > >
> > > > Fixes appveyor build reported by forcing external includes.
> > >
> > > Thanks, Alin.  I know nothing about the windows build process, but
> > > I
> > > see
> > > that this patch fixes the issue with the current AppVeyor CI,
> > > therefore:
> > >
> > > Acked-by: Ilya Maximets <i.maximets@ovn.org>
> >
> > Thank you!
> >
> > > Out of curiosity, is this change backward compatible?  I mean,
> > > is it possible to build on older platform (older VS) with this
> > > change?
> >
> > It should be.
> > Usually we do not need to force the order of include directories. For
> > kernel projects it should default to the kernel includes.
> > I test with the last two versions of VS (2019, 2017).
> >
> > We should add a build matrix for different versions of VS images to
> > appveyor / GHA so we could be sure.
> > I'll try to update the appveyor side.
> >
> > FWIW a new version of VS was launched yesterday (
> >
> https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes#visual-studio-2019-version-1610-releases
> > ), I will try to compile
> > without the patch to see if they hotfixed the issue.
>
> It did not. Applying the patch.
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Alin-Gabriel Serdean June 30, 2021, 3:46 p.m. UTC | #6
Hi Guru,

 

Thank you for letting me know. I backported the change until 2.13.

 

Thank you,

Alin.

 

From: Guru Shetty <guru.ovn@gmail.com> 
Sent: Saturday, June 26, 2021 1:14 AM
To: Alin-Gabriel Serdean <aserdean@ovn.org>
Cc: Ilya Maximets <i.maximets@ovn.org>; ovs dev <dev@openvswitch.org>; Frank Wagner <frank.wagner@dbosoft.eu>
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Specify external include paths

 

Alin,

 Does it make sense to apply this patch to stable branches too. I see that 2.15 fails with a similar error.

 

Thanks,

Guru

 

On Thu, 17 Jun 2021 at 02:31, Alin-Gabriel Serdean <aserdean@ovn.org <mailto:aserdean@ovn.org> > wrote:

On Wed, 2021-06-16 at 18:06 +0300, Alin-Gabriel Serdean wrote:
> On Tue, 2021-06-15 at 18:06 +0200, Ilya Maximets wrote:
> > On 6/15/21 3:43 PM, Alin Gabriel Serdean wrote:
> > > VStudio 16.10 adds usermode includes before including the driver
> > > kit ones.
> > > 
> > > Bug tracked at:
> > > https://developercommunity.visualstudio.com/t/error-lnk2019-unresolved-external-symbol-stdio-com/1434674
> > > 
> > > Fixes appveyor build reported by forcing external includes.
> > 
> > Thanks, Alin.  I know nothing about the windows build process, but
> > I
> > see
> > that this patch fixes the issue with the current AppVeyor CI,
> > therefore:
> > 
> > Acked-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org> >
> 
> Thank you!
> 
> > Out of curiosity, is this change backward compatible?  I mean,
> > is it possible to build on older platform (older VS) with this
> > change?
> 
> It should be.
> Usually we do not need to force the order of include directories. For
> kernel projects it should default to the kernel includes.
> I test with the last two versions of VS (2019, 2017).
> 
> We should add a build matrix for different versions of VS images to
> appveyor / GHA so we could be sure.
> I'll try to update the appveyor side.
> 
> FWIW a new version of VS was launched yesterday (
> https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes#visual-studio-2019-version-1610-releases
> ), I will try to compile
> without the patch to see if they hotfixed the issue.

It did not. Applying the patch.
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/ovsext.vcxproj b/datapath-windows/ovsext/ovsext.vcxproj
index d50a126b4..18f884f41 100644
--- a/datapath-windows/ovsext/ovsext.vcxproj
+++ b/datapath-windows/ovsext/ovsext.vcxproj
@@ -192,22 +192,39 @@ 
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win10 Release|x64'">
     <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win10 Debug|x64'">
     <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win10Analyze|x64'">
     <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
     <CodeAnalysisRuleSet>..\misc\DriverRecommendedRules.ruleset</CodeAnalysisRuleSet>
     <RunCodeAnalysis>true</RunCodeAnalysis>
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win8.1Analyze|x64'">
     <RunCodeAnalysis>true</RunCodeAnalysis>
     <CodeAnalysisRuleSet>..\misc\DriverRecommendedRules.ruleset</CodeAnalysisRuleSet>
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win8Analyze|x64'">
     <RunCodeAnalysis>true</RunCodeAnalysis>
     <CodeAnalysisRuleSet>..\misc\DriverRecommendedRules.ruleset</CodeAnalysisRuleSet>
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win8 Debug|x64'">
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win8 Release|x64'">
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win8.1 Debug|x64'">
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Win8.1 Release|x64'">
+    <ExternalIncludePath>$(CRT_IncludePath);$(KM_IncludePath);</ExternalIncludePath>
   </PropertyGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Win8 Release|x64'">
     <ClCompile>