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: windows support #42

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

jinzhongjia
Copy link
Contributor

@jinzhongjia jinzhongjia commented Jun 13, 2024

This PR is used to add windows support, but do not merge it yet.
The PR still needs more testing

@jinzhongjia
Copy link
Contributor Author

jinzhongjia commented Jun 13, 2024

I noticed that page_allocator is used extensively in the overall code. This memory allocator will directly allocate the entire page, and its speed is slower.

Now I created a new file called tool.zig, which stores an allocator. Maybe we can use gpa(GeneralPurposeAllocator)

@jinzhongjia
Copy link
Contributor Author

And I adjusted the build mode in build.zig to default, which will speed up compilation on windows and facilitate debugging.

@jinzhongjia
Copy link
Contributor Author

I also found that the use of std.debug.print seems a bit excessive. Zig officially recommends using std.log to achieve this

@jinzhongjia
Copy link
Contributor Author

and tested the following latest released zvm under my wsl2, it seems to have some issues
image

@jinzhongjia
Copy link
Contributor Author

But it seems that they are all executed successfully, and further investigation may be needed.

@jinzhongjia
Copy link
Contributor Author

Oh, I see that most of the situations above have been mentioned by you in other issues

@hendriknielaender
Copy link
Owner

hendriknielaender commented Jun 13, 2024

Oh, I see that most of the situations above have been mentioned by you in other issues

Yeah, if there is something not yet covered feel free to open up an issue.

I also found that the use of std.debug.print seems a bit excessive. Zig officially recommends using std.log to achieve this

For not getting this PR too big. You can open up a pull request for these improvements or like i mentioned open up an issue.

IIRC github action runner are using "windows-server" which it is not the same as enterprise windows. Had some trouble because of that in other projects. For the first integration that could be fine, but maybe using something like this https://hub.docker.com/_/microsoft-windows-base-os-images could be better.

Overall looking great, really appreciate your contribution. Ping me if you think it should be reviewed.

@jinzhongjia
Copy link
Contributor Author

Then if you think there is no problem, I have done some tests and it seems that we can temporarily merge the PR.

@jinzhongjia
Copy link
Contributor Author

Regarding other issues, I will open a new PR separately.

Copy link
Owner

@hendriknielaender hendriknielaender left a comment

Choose a reason for hiding this comment

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

Looks good to me! Great work.

src/alias.zig Outdated Show resolved Hide resolved
src/alias.zig Outdated Show resolved Hide resolved
jinzhongjia and others added 2 commits June 19, 2024 22:09
Co-authored-by: hndrk <51416554+hendriknielaender@users.noreply.github.com>
@jinzhongjia
Copy link
Contributor Author

The current code can only be said to work, after the PR is merged, I will reorganize the code logic, including fixes for issues that have been raised

@jinzhongjia
Copy link
Contributor Author

Later we can try to apply for administrator rights when needed. The speed of copying files on the Windows platform is really not ideal.

@hendriknielaender hendriknielaender merged commit d53b2b4 into hendriknielaender:main Jun 19, 2024
4 checks passed
@jinzhongjia jinzhongjia deleted the windows branch June 19, 2024 14:42
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.

2 participants