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

Windows CI with GitHub actions #894

Merged
merged 8 commits into from
Jun 15, 2024
Merged

Windows CI with GitHub actions #894

merged 8 commits into from
Jun 15, 2024

Conversation

mgautierfr
Copy link
Collaborator

@mgautierfr mgautierfr commented Jun 7, 2024

Fixes #891

This PR mainly:

This current version generate a lot of warning on windows at compilation about stl structure not exported. As said in the commit message, it seems we may ignore them. @veloman-yunkan, your input would be valuable on this.

Note that we still compile libzim without xapian (the current status on this is that we succeed to compile xapian on windows but unit-tests are failing)

(This PR is mostly the same than #892 but with a branch named as it sister branch on kiwix-build. This way we use its dev dependency archive)

.github/workflows/ci.yml Dismissed Show dismissed Hide dismissed
@mgautierfr mgautierfr force-pushed the libzim_github_ci_windows branch 6 times, most recently from f1bea40 to cb78cc2 Compare June 7, 2024 13:08
`static_linkage` is about how we link with dependency libraries.
We must set `LIBZIM_EXPORT_DLL` depending of how we build libzim library.
@mgautierfr mgautierfr marked this pull request as ready for review June 10, 2024 13:01
include/zim/archive.h Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@mgautierfr mgautierfr force-pushed the libzim_github_ci_windows branch 3 times, most recently from 465754b to 4d536ce Compare June 14, 2024 14:41
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

LGTM. Rebase & fixup is the next and last step before merging.

While they are internal (and not part of public API), unittest are testing
them and so we need to link to them.
On Windows, static members are not exported in dll.
So we must not use them in inlined method/constructor.
On Windows, if we declare a exported class, the implementation of this
class is expected in the dll.

When compiling without xapian we must not include headers for which we
don't compile the implementation.
MSVC throw a lot of warning because we also must re-export stl symbols
used in our exported class.

See [1] and [2] (and links inside) for more information.

Especially [3] which seems to say that we should have issue at link time if
it compatibility issue occurs (and so easily catchable).

So I just remove the `werror=true` for now.

[1] https://stackoverflow.com/questions/16419318/one-way-of-eliminating-c4251-warning-when-using-stl-classes-in-the-dll-interface
[2] https://stackoverflow.com/questions/2132747/warning-c4251-when-building-a-dll-that-exports-a-class-containing-an-atlcstrin
[3] https://stackoverflow.com/questions/2132747/warning-c4251-when-building-a-dll-that-exports-a-class-containing-an-atlcstrin#comment66255284_4563701
@mgautierfr
Copy link
Collaborator Author

Done. Commit message about adding LIBZIM_API to private api has been updated to LIBZIM_PRIVATE_API

@kelson42 kelson42 merged commit 8b241bb into main Jun 15, 2024
34 checks passed
@kelson42 kelson42 deleted the libzim_github_ci_windows branch June 15, 2024 14:39
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.

Windows CI using Github actions
3 participants