Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub Actions: add testing for Windows Vistual Studio / nmake #3806

Conversation

leleliu008
Copy link
Member

No description provided.

Signed-off-by: leleliu008 <leleliu008@gmail.com>
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d104ee5) 85.06% compared to head (fb02761) 85.06%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3806   +/-   ##
=======================================
  Coverage   85.06%   85.06%           
=======================================
  Files         226      226           
  Lines       53857    53857           
=======================================
  Hits        45816    45816           
  Misses       8041     8041           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@masatake
Copy link
Member

@k-takata Could you review this?

Copy link
Member

@k-takata k-takata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is okay for the first step.

For the next step, we should run misc/units.py on msys2 as we already to in AppVeyor.

I'm thinking if we can use vcpkg in the future to set up external libraries like iconv.

@leleliu008
Copy link
Member Author

libyaml libxml2 jansson all support cmake build system, cmake is supported as of Virsual Studio 2017
https://devblogs.microsoft.com/cppblog/cmake-support-in-visual-studio-the-visual-studio-2017-rc-update/

These packages could be easily built with cmake even if do not use any package managers.

@masatake
Copy link
Member

Do you know if we can use C11? I frequently want to use anonymous union and struct.

@k-takata
Copy link
Member

MS document about C11 and C17 is here:
https://learn.microsoft.com/en-us/cpp/overview/install-c17-support?view=msvc-170
Also related:
https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-170

I frequently want to use anonymous union and struct.

This has been supported in MSVC for more than 25 years, I think.

@leleliu008
Copy link
Member Author

per https://learn.microsoft.com/en-us/cpp/overview/install-c17-support?view=msvc-170

Support for C11 and C17 standards is available in Visual Studio 2019 version 16.8 and later. 

Support requires an updated Universal C Runtime (UCRT) and Windows SDK version to work properly with the conforming preprocessor ([/Zc:preprocessor](https://learn.microsoft.com/en-us/cpp/build/reference/zc-preprocessor?view=msvc-170)).

If we do support the latest two releases of Vistual Studio (Vistual Studio 2022 and Vistual Studio 2019 at this point), using C11 should have no problem.

@leleliu008
Copy link
Member Author

This has been supported in MSVC for more than 25 years, I think.

@k-takata

You mean anonymous union and struct had been suppoted by Vistual Studio prior to C11?

I have no windows programming experience at all.

as per these documentations:

https://learn.microsoft.com/en-us/cpp/c-language/structure-declarations?view=msvc-140
https://learn.microsoft.com/en-us/cpp/c-language/structure-declarations?view=msvc-150
https://learn.microsoft.com/en-us/cpp/c-language/structure-declarations?view=msvc-160
https://learn.microsoft.com/en-us/cpp/c-language/structure-declarations?view=msvc-170

It seems that anonymous union and struct already have been suppoted by Vistual Studio many years ago.

@masatake
Copy link
Member

@leleliu008, let's merge this.

@leleliu008 leleliu008 merged commit 713c557 into universal-ctags:master Aug 24, 2023
42 checks passed
@masatake
Copy link
Member

Thank you. I'll make a pull request that uses the "anonymous struct and union" of C11 and see what happens.

@k-takata
Copy link
Member

You mean anonymous union and struct had been suppoted by Vistual Studio prior to C11?

Yes.

@masatake
Copy link
Member

@leleliu008 @k-takata Can we remove the combination

    - compiler: msvc
      ARCH: x64

from appveyor.yml now?

I would like to work on #3795 after making the pull request pass all the testing.

@leleliu008
Copy link
Member Author

@leleliu008
Copy link
Member Author

leleliu008 commented Sep 15, 2023

If you just want to pass the workflows on appveyor, I think applying following patch is the easiest way:

diff --git a/appveyor.yml b/appveyor.yml
index edd8c5ae..f8a38380 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -16,6 +16,7 @@ environment:
     #   MSYSTEM: MINGW32
     - compiler: msvc
       ARCH: x64
+      APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
     #- compiler: msvc
     #  ARCH: x86
     - compiler: mingw

Reference: https://www.appveyor.com/docs/build-environment/#choosing-image-for-your-builds

If the build configuration does not specify build worker image then Visual Studio 2015 image is used.

our appveyor.yml doesn't specify build worker image, so Visual Studio 2015 image is used. We could explicitly tell appveyor that we want to use Visual Studio 2019 or Visual Studio 2022

@leleliu008
Copy link
Member Author

leleliu008 commented Sep 15, 2023

@masatake
I was gonna tweak appveyor.yml and win32/appveyor.bat via PR (#3815), but it fails due to some compiler warnings. I have no idea what's going on, GitHub Actions use Visual Studio 2019 Enterprise has no warnings. AppVeyor use Visual Studio 2019 Community reports some warnings, due to /WX compiler option is used, these warrnings are treated as errors and abort the build process.

@leleliu008 @k-takata Can we remove the combination

    - compiler: msvc
      ARCH: x64

from appveyor.yml now?

I would like to work on #3795 after making the pull request pass all the testing.

I think msvc-x64 environment for appveyor can be removed. running units on MSYS2 had been added via PR (#3817) just now.

@masatake
Copy link
Member

I was gonna tweak appveyor.yml and win32/appveyor.bat via PR (#3815), but it fails due to some compiler warnings. I have no idea what's going on, GitHub Actions use Visual Studio 2019 Enterprise has no warnings.

It seems that the code generated by packcc causes the warnings. I'll look into it.

@leleliu008
Copy link
Member Author

I was gonna tweak appveyor.yml and win32/appveyor.bat via PR (#3815), but it fails due to some compiler warnings. I have no idea what's going on, GitHub Actions use Visual Studio 2019 Enterprise has no warnings.

It seems that the code generated by packcc causes the warnings. I'll look into it.

What I'm confused is that GitHub Actions and AppVeyor both use Visual Studio 2019, the only difference is that the former is Enterprise Edition, the later is Community Edition

@k-takata
Copy link
Member

What I'm confused is that GitHub Actions and AppVeyor both use Visual Studio 2019, the only difference is that the former is Enterprise Edition, the later is Community Edition

The AppVeyor job is compiled with -DDEBUG and the GHA job is compiled without -DDEBUG.
Line 133 of peg_common.h is compiled only when DEBUG is defined.

@leleliu008
Copy link
Member Author

@k-takata Thank you very much for the hint.

@leleliu008
Copy link
Member Author

It seems that the code generated by packcc causes the warnings. I'll look into it.

It seems that peg\peg_common.h is not generated by packcc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants