Skip to content

resinator should not observe any environment variables #17585

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

Closed
andrewrk opened this issue Oct 18, 2023 · 2 comments · Fixed by #17608
Closed

resinator should not observe any environment variables #17585

andrewrk opened this issue Oct 18, 2023 · 2 comments · Fixed by #17608
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

One of zig's principles as a toolchain is to reduce dependencies on global state of the system, so that collaborators on wildly different systems can have a seamless experience working with each other.

To that end, this code should be deleted:

if (!options.ignore_include_env_var) {
const INCLUDE = std.process.getEnvVarOwned(allocator, "INCLUDE") catch "";
defer allocator.free(INCLUDE);
// TODO: Should this be platform-specific? How does windres/llvm-rc handle this (if at all)?
var it = std.mem.tokenize(u8, INCLUDE, ";");
while (it.next()) |search_path| {
var dir = openSearchPathDir(options.cwd, search_path) catch continue;
errdefer dir.close();
try search_dirs.append(.{ .dir = dir, .path = try allocator.dupe(u8, search_path) });
}
}

All include paths that are intended to be observed by resinator should be provided explicitly by the user, and explicitly for resinator and no other files, and it should be per invocation rather than global.

I noticed this when enhancing zig env to tattle on what environment variables are possibly observed by the compiler, and it's unfortunate that INCLUDE is in this list.

cc @squeek502

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Oct 18, 2023
@andrewrk andrewrk added this to the 0.12.0 milestone Oct 18, 2023
@squeek502
Copy link
Collaborator

squeek502 commented Oct 18, 2023

EDIT: Oops, yes, this was an accidental regression in the clang argv setup in #17412 (I don't pass ignore_include_env_var = true to it).

Will fix this tomorrow.


Unless I accidentally regressed something (very possible), this is already the case for everything except zig rc:

ignore_include_env_var is unconditionally set to true when compiling .rc and .manifest files:

.ignore_include_env_var = true,

.ignore_include_env_var = true,

It is controlled by the /x flag in zig rc for compatibility with rc.exe but I could be persuaded against that if you feel it would be an improvement (the INCLUDE env var is only really set when setting up a "Visual Studio development environment" via the vcvarsall.bat and friends scripts, and resinator doesn't really depend on it being set since it autodetects the system include paths by default anyway).

@andrewrk
Copy link
Member Author

Nice, that all makes sense and I think your intended way for it to work is perfectly reasonable.

squeek502 added a commit to squeek502/zig that referenced this issue Oct 19, 2023
The INCLUDE variable being used during preprocessing was an accidental regression caused by ziglang#17412.

Closes ziglang#17585.
squeek502 added a commit to squeek502/zig that referenced this issue Oct 19, 2023
The INCLUDE variable being used during `.rc` preprocessing was an accidental regression in ziglang#17412.
Closes ziglang#17585.

resinator changes:
source_mapping: Protect against NUL bytes in #line filenames
lex: Avoid recalculating column on every tab stop within string literals
Proper error handling for failing to open cwd instead of `catch unreachable`
Use platform-specific delimiter for INCLUDE env var parsing
@andrewrk andrewrk added the accepted This proposal is planned. label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants