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

feat(CI): Windows MSVC and GCC build #6015

Merged
merged 9 commits into from
Apr 18, 2024

Conversation

liamHowatt
Copy link
Collaborator

@liamHowatt liamHowatt commented Apr 3, 2024

Description of the feature or fix

See #5904 and #5978

Add a CI test for building on Windows. In particular, build the Windows-specific files from #5313.

Notes

@kisvegabor
Copy link
Member

Very nice! It would be great to reuse the CMake files for MSVC too, but if there is more Windows-ish way we can choose that too.

@MouriNaruto could you take a look at the Windows driver related changes?

@MouriNaruto
Copy link
Contributor

MouriNaruto commented Apr 4, 2024

@kisvegabor I have read the changes. I think @liamHowatt should move the device window message handler declarations back because these are private interfaces.

For LVGL Windows backend, I try my best to maintain the ABI compatibility. So, I don't hope to see making some private interfaces to be public. (The lv_windows_input.h and lv_windows_display.h only contain the public API. The lv_windows_context.h only contains the context-related private APIs.)

Kenji Mouri

@liamHowatt
Copy link
Collaborator Author

Thanks for the feedback @MouriNaruto. In hindsight I see that those handlers should not be the header as you say. I originally did it to fix a CI warning.

https://github.com/liamHowatt/lvgl/actions/runs/8527506893/job/23359087388

Error: D:/a/lvgl/lvgl/src/drivers/windows/lv_windows_input.c:283:6: error: no previous prototype for 'lv_windows_pointer_device_window_message_handler' [-Werror=missing-prototypes]
  283 | bool lv_windows_pointer_device_window_message_handler(
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Generally it's good to only-declare-once. Recently we've been moving private cross-file declarations to *_private.h headers #5913. What do you think of my latest commit(s) as a potential compromise?

@kisvegabor
Copy link
Member

If @MouriNaruto agrees with having lv_windows_input_private.h, please add it to lvgl_private.h too.

@MouriNaruto
Copy link
Contributor

@kisvegabor I'm agree with having lv_windows_input_private.h.

XuNeo
XuNeo previously approved these changes Apr 6, 2024
Copy link
Collaborator

@XuNeo XuNeo left a comment

Choose a reason for hiding this comment

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

Nice work!

@MouriNaruto
Copy link
Contributor

@liamHowatt I think you should do some macro checks for GCC-specific progma to keep the MSVC compatibility.

Kenji Mouri

@kisvegabor
Copy link
Member

Will you migrate to MSVC still in this PR?

@liamHowatt
Copy link
Collaborator Author

I think you should do some macro checks for GCC-specific progma to keep the MSVC compatibility.

Yep, good idea.

Will you migrate to MSVC still in this PR?

That wasn't the intent of this PR but yes I will. It is important.

@kisvegabor
Copy link
Member

Will you migrate to MSVC still in this PR?

That wasn't the intent of this PR but yes I will. It is important.

Yes, I think MinGW is not so interesting compared to MSVC.

@liamHowatt
Copy link
Collaborator Author

There is now an MSVC build workflow.

  • Using /W3 warning flag to enable all "production quality" warnings. Not using /W4 because it gives too many Spectre vulnerability mitigation warnings.
  • Manually set warning C4013 to be treated as an error. It is the "implicit function declaration" warning.
  • The warnings from chore: change position and size parameters to size_t to avoid 64-bit system compiler warnings #5873 are being emitted.
  • I cleaned up some concerning warnings but left most of them alone for after 5873 gets merged. There are a lot of integer conversion size/sign warnings as well as signed/unsigned comparison warnings. build logs
  • ASM disabled for MSVC in CMake

@liamHowatt liamHowatt changed the title feat(CI): Windows MinGW GCC build feat(CI): Windows MSVC and GCC build Apr 18, 2024
@liamHowatt liamHowatt requested a review from XuNeo April 18, 2024 04:49
@kisvegabor
Copy link
Member

Thank you!

At some point it would be nice to lower the number of the remaining warnings, but at least now we know that LVGL builds on MSVC too! 🎉

@kisvegabor kisvegabor merged commit e72f52d into lvgl:master Apr 18, 2024
18 checks passed
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.

None yet

4 participants