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

std.zig: search include dir and lib dir from environment variables #13145

Merged
merged 4 commits into from Oct 16, 2022

Conversation

igaryhe
Copy link
Contributor

@igaryhe igaryhe commented Oct 12, 2022

distro like guix doesn't use FHS, so it relies on envorinment variables (C_INCLUDE_PATH, CPLUS_INCLUDE_PATH and LIBRARY_PATH) to search for headers and libraries

@igaryhe igaryhe force-pushed the nativepath-envvar branch 3 times, most recently from 78a0867 to 48d0fe6 Compare October 13, 2022 05:13
lib/std/zig/system/NativePaths.zig Outdated Show resolved Hide resolved
lib/std/zig/system/NativePaths.zig Outdated Show resolved Hide resolved
@igaryhe igaryhe requested review from marler8997 and squeek502 and removed request for marler8997 and squeek502 October 13, 2022 10:49
@igaryhe igaryhe requested review from N00byEdge and removed request for marler8997 October 13, 2022 17:04
@igaryhe igaryhe changed the title search include dir and lib dir from environment variables std.zig: search include dir and lib dir from environment variables Oct 13, 2022
@marler8997
Copy link
Contributor

Another thought comes to mind...since this code doesn't apply to Windows, we could just use os.getenv, which avoids needing an allocator, needing to copy the value, the OutOfMemory error and the InvalidUTF8 error.

@igaryhe
Copy link
Contributor Author

igaryhe commented Oct 14, 2022

Another thought comes to mind...since this code doesn't apply to Windows, we could just use os.getenv, which avoids needing an allocator, needing to copy the value, the OutOfMemory error and the InvalidUTF8 error.

lol at some point i also thought about it but i'm not sure if the implementation is correct, so i just stick with process.getEnvVarOwned. it seems more robust indeed

@andrewrk andrewrk merged commit ca1c185 into ziglang:master Oct 16, 2022
@andrewrk
Copy link
Member

Nice work everyone!

@igaryhe igaryhe deleted the nativepath-envvar branch October 16, 2022 16:11
@jcmoyer
Copy link
Contributor

jcmoyer commented Oct 16, 2022

This seems to be broken on Windows because it triggers #8456 for me. Compiling from 5127dae with:

LLD Link... lld-link: error: undefined symbol: environ
>>> referenced by D:\scratch\zig\zig-cache\o\cbc889a12c46d73c2ad4c4cda0071cee\zig.exe.obj:(.refptr.environ)
error: FileNotFound
error: the following build command failed with exit code 1:
D:\scratch\zig\zig-cache\o\357e5a72efda7ba918afe0b3717f4f9b\build.exe D:\Scratch\zig-install\bin\zig.exe D:\scratch\zig D:\scratch\zig\zig-cache C:\Users\jcmoyer\AppData\Local\zig -p stage3 --search-prefix D:\Scratch\zig+llvm+lld+clang-x86_64-windows-gnu-0.10.0-dev.4300+1f196b9e2 -Denable-stage1 -Dstatic-llvm --zig-lib-dir lib -Duse-zig-libcxx -Dtarget=x86_64-windows-gnu

>zig env
{
 "zig_exe": "D:\\Scratch\\zig-install\\bin\\zig.exe",
 "lib_dir": "D:\\Scratch\\zig-install\\lib",
 "std_dir": "D:\\Scratch\\zig-install\\lib\\std",
 "global_cache_dir": "C:\\Users\\jcmoyer\\AppData\\Local\\zig",
 "version": "0.10.0-dev.4334+5d520454e",
 "target": "x86_64-windows.win10_fe...win10_fe-gnu"
}

E: Builds fine after reverting only ca1c185

E2: It may be because this line isn't comptime like the above branches https://github.com/igaryhe/zig/blob/348935988ecec3f3af076b6091ffefebed673a09/lib/std/zig/system/NativePaths.zig#L110

@igaryhe
Copy link
Contributor Author

igaryhe commented Oct 16, 2022

@jcmoyer it's strange, i just tried compiling locally from 5127dae with tho following commands:

zig build --prefix stage3-release --search-prefix ..\zig_deps --zig-lib-dir lib -Denable-stage1 -Dstatic-llvm -Drelease -Dstrip -Duse-zig-libcxx -Dtarget=x86_64-windows-gnu

and it didn't show any error. and my zig env shows

{
 "zig_exe": "D:\\scoop\\apps\\zig-dev\\current\\zig.exe",
 "lib_dir": "D:\\scoop\\apps\\zig-dev\\current\\lib",
 "std_dir": "D:\\scoop\\apps\\zig-dev\\current\\lib\\std",
 "global_cache_dir": "C:\\Users\\dan\\AppData\\Local\\zig",
 "version": "0.10.0-dev.4333+f5f28e0d2",
 "target": "x86_64-windows.win10_fe...win10_fe-gnu"
}

i assume the only different is the zig version?

the result from the ci also shows a successful build.

but you might be right, the line you mentioned should be comptime.

have to say i'm not familiar with the zig code other than the lines i contributed, would like to hear from @andrewrk

@jcmoyer
Copy link
Contributor

jcmoyer commented Oct 16, 2022

@igaryhe The CI printed a message about it here, not sure why it didn't fail though. Your build command seems almost identical to mine except I'm building a debug mode binary.

@squeek502
Copy link
Collaborator

I wonder why the native_info parameter isn't just marked comptime. Doesn't the if (comptime native_target.os.tag == .solaris) { check force it to always be comptime anyway?

@igaryhe
Copy link
Contributor Author

igaryhe commented Oct 17, 2022

i think it's better for us to open a new issue to track this problem

@igaryhe
Copy link
Contributor Author

igaryhe commented Oct 17, 2022

@jcmoyer ok, just got this tested, and when building in debug mode, lld throws the error; but with the -Drelease flag, it build successfully. it's weird because i copied the build command from the ci's build script:

& "$ZIGPREFIXPATH\bin\zig.exe" build `
--prefix "$ZIGINSTALLDIR" `
--search-prefix "$ZIGPREFIXPATH" `
--zig-lib-dir "$ZIGLIBDIR" `
-Denable-stage1 `
-Dstatic-llvm `
-Drelease `
-Dstrip `
-Duse-zig-libcxx `
-Dtarget=$(TARGET)

but that might because it's actually the output from the test phase, where it's tested in debug mode:
& "$ZIGINSTALLDIR\bin\zig.exe" build test docs `
--search-prefix "$ZIGPREFIXPATH" `
-Dstatic-llvm `
-Dskip-non-native

adding the comptime keyword to line 110 doesn't solve the lld error. a temporary fix might be falling back to the previously implemented process.getEnvVarOwned function, but at the moment i would assume it might be a compiler bug and need further investigation.

i'll open a new issue for this, and hope contributors could help us look into this problem.

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

6 participants