-
Notifications
You must be signed in to change notification settings - Fork 396
Added a simple compilation test and linux CI (and fix linux build) #446
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
Conversation
Compilation test is required because the smaple is windows-specific. Linux CI is needed because pull requests must stop breaking linux builds.
include/vk_mem_alloc.h
Outdated
return allocation->GetWin32Handle(allocator, hTargetProcess, pHandle); | ||
} | ||
#endif // VMA_EXTERNAL_MEMORY_WIN32 | ||
#endif // VMA_EXTERNAL_MEMORY_WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that you change many lines that are unrelated to your change due to using some automated formatting tools. Changes like this make it difficult to analyze the history of a file later (like "blame" function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mrkol Let me know if you need help doing this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to reuse VulkanSample app for testing compilation on Linux instead of creating a new project? VulkanSample.cpp and VmaUsage.* files already have appropriate #ifdef
-s and even some Clang-specific #pragma
directives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we have to port VulkanSample app to Linux? I would be happy to help, but note that this would require glfw library most likely (#169).
Does this pull request mean you want to move away from appveyor and use just GitHub actions instead? I could help with that. In my opinion, it would be nice to have a CI for Windows and Linux and both Debug and Release. |
@IAmNotHanni Help is very welcome. It would be good to have CI builds set up for both Windows and Linux using some free online service. We had AppVeyor for Windows and TravisCI for Linux. Unfortunately, TravisCI stopped working >2 years ago. AppVeyor is still testing Windows. It is enough to test if the library compiles successfully (both public interface and the code under Running some actual tests in CI could be considered in the future, but that would require access to some Vulkan implementation on the server (a GPU or software emulation) and likely major changes in the sample app and testing code, which I would prefer to avoid. The problem is I use Windows at work and I don't have a local Linux development environment to be able to test and fix things. |
The proposed code fails with error:
|
Use
|
File ".github/workflows/linux.yaml" added by this change uses "actions/checkout@v4". I think it's the external repository "humbletim/[email protected]" referenced by this change that triggers this error. I think we should just download and install Vulkan SDK (or just its headers) without referencing such external repository. |
Yes, it should be downloaded directly like this for example:
|
@adam-sawicki-a Looking at the recent changes you pushed to master branch (0183545), one thing which points out to me is that clang and gcc seem to display no warnings at all in the CI build. In my experience, gcc and clang are much more detailed with warnings and they usually catch things which MSVC does not report by default. Maybe flags like |
Yes, this sounds like a good idea as long as those warnings don't fail the build. Can you please propose such change? |
Also, don't forget that in VmaSample code, there are a lot of |
I added Linux CI build as #477, based on this ticket. Thank you very much for your help with it. |
Yes, let me see.. |
Yes, the library is compiled. I made a syntax error in "vk_mem_alloc.h" to check if the build fails - it does. |
Trivial compilation test is required because the sample is windows-specific. Linux CI is needed because pull requests must stop breaking linux builds. The tiny
m_Handle(VMA_NULL)
->m_Handle()
fix is needed to fix linux CI.As promised @adam-sawicki-a . Same day delivery because my CI is still broken =)
(I will stop using master for my builds soon, I promise)