Skip to content

Conversation

@binary132
Copy link

@binary132 binary132 commented Jun 18, 2018

Issue: #1117

The macOS stage1 Zig compiler should look in Zig's real absolute path
for the Zig stdlib, but os_self_exe_path looks in its path as returned
by _NSGetExecutablePath, which may be a symlink. This means that a
symlinked Zig cannot find the Zig stdlib.

This patch fixes the issue by resolving the _NSGetExecutablePath result
to the real path using realpath() before copying the result to the
output path.

src/os.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Instead of resolve_tmp, we should resize out_path and use it directly. And then we need to buf_resize(out_path, strlen(buf_ptr(out_path))) to make the length correct.

Can you confirm this patch works for you?

Copy link
Author

Choose a reason for hiding this comment

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

Works for me.

@andrewrk
Copy link
Member

Thanks for the patch, one little fixup and then it should be good to merge.

Issue: ziglang#1117

The macOS stage1 Zig compiler should look in Zig's real absolute path
for the Zig stdlib, but os_self_exe_path looks in its path as returned
by _NSGetExecutablePath, which may be a symlink.  This means that a
symlinked Zig cannot find the Zig stdlib.

This patch fixes the issue by resolving the _NSGetExecutablePath result
to the real path using realpath() before copying the result to the
output path.
@binary132 binary132 force-pushed the fix-1117-macos-realpath branch from 0fa01b4 to c7057bd Compare June 18, 2018 11:41
@andrewrk andrewrk merged commit c7057bd into ziglang:master Jun 18, 2018
buf_resize(out_path, u32_len);
int ret2 = _NSGetExecutablePath(buf_ptr(out_path), &u32_len);

// Make a buffer having room for the temp path.
Copy link
Contributor

@bnoordhuis bnoordhuis Jun 18, 2018

Choose a reason for hiding this comment

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

This might just be me but I personally don't really like comments that just say what the code on the line below does.

As a rule of thumb, comments should explain what won't be obvious from just looking at the code, not restate the code in prose.

edit: hadn't noticed this has been merged between starting and finishing the review.

Copy link
Author

Choose a reason for hiding this comment

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

The justification is that it makes code faster to read.

assert(ret2 == 0);

// Resolve the real path from that.
buf_resize(out_path, PATH_MAX);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not relevant to Zig but in libuv we allocate 2*PATH_MAX to work around a bug in old versions of Apple's libc where the expanded path is sometimes > PATH_MAX.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I'll throw that in

Copy link
Member

Choose a reason for hiding this comment

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

andrewrk added a commit that referenced this pull request Jun 18, 2018
@binary132 binary132 deleted the fix-1117-macos-realpath branch June 19, 2018 11:30
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.

None yet

4 participants