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

Add meson build system support #525

Merged
merged 12 commits into from
Oct 6, 2024
Merged

Add meson build system support #525

merged 12 commits into from
Oct 6, 2024

Conversation

mochaaP
Copy link
Contributor

@mochaaP mochaaP commented Sep 15, 2024

Requires zyantific/zycore-c#75.

Should've reached feature parity with CMake. Please let me know if I missed anything.

Doxyfile.in Outdated
Copy link
Member

Choose a reason for hiding this comment

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

We already have a Doxyfile in the Zydis repository. Can we reuse this here? It seems that we are just using the default in Zycore. Optimally we'd make it consistent between both repos. If you can think of a way to reuse the existing Doxyfile here, please feel free to also add one based on the Zydis Doxyfile to Zycore.

Copy link
Contributor Author

@mochaaP mochaaP Sep 15, 2024

Choose a reason for hiding this comment

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

The existing Doxyfile needs to be parsed by a simple CMake function, and thus we can't add things like @DEFINES@, "quoted strings" and feature detection.

Another reason it could not be unified is that there are some minor differences between the repo (e.g. what macros to or not to expand, should add external includes or not).

I made it much simpler in 7272cde. It's now only the diffs with the default config.

Copy link
Member

@athre0z athre0z Sep 15, 2024

Choose a reason for hiding this comment

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

Thank you, that looks a lot more managable! Doxygen allows including another config as a basis via @INCLUDE = config_file_name. Can we perhaps use this to further deduplicate? I think I'd also prefer if we could rename Doxygen.in to Doxygen.meson.in to communicate the purpose of the extra file more clearly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but I think this might bring some confusion, especially when someone uses in-tree build. (however, rare to see these days, so we could ignore that scenario.)

@mochaaP mochaaP marked this pull request as ready for review September 19, 2024 23:43
@mochaaP
Copy link
Contributor Author

mochaaP commented Sep 19, 2024

Should be ready.

Copy link
Member

@athre0z athre0z left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the effort! This good to me overall, just some nits and questions. Also, sorry for the delay to review; I ended up being crazy busy in the past few weeks.

.github/workflows/main.yml Outdated Show resolved Hide resolved
dependencies/zycore.wrap Show resolved Hide resolved
subprojects Outdated
Copy link
Member

Choose a reason for hiding this comment

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

What is the symlink situation on Windows these days? It used to be a problem to have symlinks in git repos in the past. Seems that it is supported OK since some Windows 10 version? Does it have a reasonable user-experience, e.g. is the symlink marked as such in MS Explorer?

CC @Mattiwatti

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should work well (iirc it shows as .symlink in explorer on Windows 8+)

Copy link
Member

Choose a reason for hiding this comment

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

I did some research and it seems like it's still not possible to create symlinks on Windows 10 22H2 without administrator privileges by default.

Prior to Windows 10, the SeCreateSymbolicLinkPrivilege privilege must be granted to e.g. the Everyone group to allow the creation of symlinks without administrator privileges.

Since Windows 10, "Developer Mode" can be enabled to achieve the same.

I assume most developers on Windows have "Developer Mode" enabled, so from my side it would be fine to keep the symlink.

It looks like this after cloning the repo:
257-136-max

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for checking! It's a bit of a recipe for confusion to make that behavior vary depending on developer mode, but that's MS fault and not ours. Let's merge and see if we receive issues from people running into this.

@mochaaP mochaaP requested a review from athre0z October 6, 2024 06:57
Copy link
Member

@athre0z athre0z left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again!

@athre0z athre0z merged commit 9d298eb into zyantific:master Oct 6, 2024
24 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.

3 participants