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

some fixs and optimze #54

Merged
merged 6 commits into from
Jul 27, 2024

Conversation

jinzhongjia
Copy link
Contributor

@jinzhongjia jinzhongjia commented Jul 27, 2024

  • fix: use std.fs.accessAbsolute to replace std.fs.openDirAbsolute
    This can solve the problem of file descriptors not being freed
  • fix: verify zig version failed
    When the user has installed a higher zig, zvm verify zig version will failed, because the child process will use system environment variables
  • optimize: simplify the logic of set_zig_version
  • optimize: improve performance by using comptime
    rename origin detect func in architecture.zig to platform_str and make it become comptime func

This can solve the problem of file descriptors not being freed
When the user has installed a higher zig, zvm verify zig version will failed, because the child process will use system environment variables
@jinzhongjia
Copy link
Contributor Author

jinzhongjia commented Jul 27, 2024

And I delete this code

        std.posix.symlink(zig_path, symlink_path) catch |err| switch (err) {
            error.PathAlreadyExists => {
                try std.fs.cwd().deleteFile(symlink_path);
                try std.posix.symlink(zig_path, symlink_path);
            },
            else => return err,
        };

replace with this:

try std.posix.symlink(zig_path, symlink_path);

I think it's not necessary to retry the deletion in case of an error

@jinzhongjia jinzhongjia changed the title some fixs some fixs and optimze Jul 27, 2024
rename origin `detect` func in `architecture.zig` to `platform_str` and make it become comptime func
@hendriknielaender
Copy link
Owner

And I delete this code

        std.posix.symlink(zig_path, symlink_path) catch |err| switch (err) {
            error.PathAlreadyExists => {
                try std.fs.cwd().deleteFile(symlink_path);
                try std.posix.symlink(zig_path, symlink_path);
            },
            else => return err,
        };

replace with this:

try std.posix.symlink(zig_path, symlink_path);

I think it's not necessary to retry the deletion in case of an error

error.PathAlreadyExists => {
                try std.fs.cwd().deleteFile(symlink_path);
                try std.posix.symlink(zig_path, symlink_path);
            },

IIRC I had a problem in the past, when there was already a symlink. So I'm not sure about this change.
11b3199

Let me try to reproduce it.

@hendriknielaender
Copy link
Owner

improve performance by using comptime

That's cool, we should use it wherever its possible 👍

@jinzhongjia
Copy link
Contributor Author

Expectedly, the symlink should be removed in the above function

@hendriknielaender
Copy link
Owner

Expectedly, the symlink should be removed in the above function

True, overlooked it! 👍

@hendriknielaender hendriknielaender merged commit 3e610d3 into hendriknielaender:main Jul 27, 2024
4 checks passed
@jinzhongjia jinzhongjia deleted the optimize branch July 28, 2024 00:48
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