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.

Sign up for GitHub

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

Jump to bottom

resinator: Include directories regression in build system integration #19605

Closed
squeek502 opened this issue Apr 10, 2024 · 0 comments · Fixed by #19843
Closed

resinator: Include directories regression in build system integration #19605

squeek502 opened this issue Apr 10, 2024 · 0 comments · Fixed by #19843
Assignees
@squeek502
Labels
bug Observed behavior contradicts documented or intended behavior regression It worked in a previous version of Zig, but stopped working.
Milestone
0.13.0

Comments

@squeek502
Copy link
Collaborator

squeek502 commented Apr 10, 2024 •
edited
Loading

Zig Version

0.12.0-dev.3594+355cceebc

Steps to Reproduce and Observed Behavior

Before #19174, the resinator preprocessor was clang and was invoked after calling addCCArgs which added mod.cc_argv and comp.global_cc_argv to the preprocessor argv. This meant that include directories added via things like std.Build.Compile.addIncludePath would make it into the argv of the preprocessor.

After #19174, that is no longer the case. Because of this, there's no real way to use a LazyPath for include directories that get passed to resinator (there's only RcSourceFile.flags which is a []const []const u8).

Expected Behavior

Either:

  • There should be a dedicated way to interface with resinator include dirs, etc through the build system (i.e. addRcIncludePath), or
  • The include dirs added via std.Build.Compile.addIncludePath, etc should be inherited by resinator (as was done with clang before)

I'm currently leaning towards the second option, but am not totally sure yet.
@squeek502 squeek502 added bug Observed behavior contradicts documented or intended behavior regression It worked in a previous version of Zig, but stopped working. labels Apr 10, 2024
@squeek502 squeek502 self-assigned this Apr 10, 2024
@jayrod246 jayrod246 mentioned this issue Apr 10, 2024
Merged
@Vexu Vexu added this to the 0.14.0 milestone Apr 11, 2024
squeek502 added a commit to squeek502/zig that referenced this issue May 3, 2024
@squeek502
Build system: Allow specifying Win32 resource include paths using Laz…
c8c5662 
…yPath

Adds an `include_paths` field to RcSourceFile that takes a slice of LazyPaths. The paths are resolved and subsequently appended to the -rcflags as `/I <resolved path>`.

This fixes an accidental regression from ziglang#19174. Before that PR, all Win32 resource compilation would inherit the CC flags (via `addCCArgs`), which included things like include directories. After that PR, though, that is no longer the case.

However, this commit intentionally does not restore the previous behavior (inheriting the C include paths). Instead, each .rc file will need to have its include paths specified directly and the include paths only apply to one particular resource script. This allows more fine-grained control and has less potentially surprising behavior (at the cost of some convenience).

Closes ziglang#19605
@squeek502 squeek502 mentioned this issue May 3, 2024
Merged
@andrewrk andrewrk modified the milestones: 0.14.0, 0.13.0 May 3, 2024
andrewrk pushed a commit that referenced this issue May 3, 2024
@squeek502 @andrewrk
Build system: Allow specifying Win32 resource include paths using Laz…
ea9d817 
…yPath

Adds an `include_paths` field to RcSourceFile that takes a slice of LazyPaths. The paths are resolved and subsequently appended to the -rcflags as `/I <resolved path>`.

This fixes an accidental regression from #19174. Before that PR, all Win32 resource compilation would inherit the CC flags (via `addCCArgs`), which included things like include directories. After that PR, though, that is no longer the case.

However, this commit intentionally does not restore the previous behavior (inheriting the C include paths). Instead, each .rc file will need to have its include paths specified directly and the include paths only apply to one particular resource script. This allows more fine-grained control and has less potentially surprising behavior (at the cost of some convenience).

Closes #19605
andrewrk pushed a commit that referenced this issue May 22, 2024
@squeek502 @andrewrk
Build system: Allow specifying Win32 resource include paths using Laz…
ddde99b 
…yPath

Adds an `include_paths` field to RcSourceFile that takes a slice of LazyPaths. The paths are resolved and subsequently appended to the -rcflags as `/I <resolved path>`.

This fixes an accidental regression from #19174. Before that PR, all Win32 resource compilation would inherit the CC flags (via `addCCArgs`), which included things like include directories. After that PR, though, that is no longer the case.

However, this commit intentionally does not restore the previous behavior (inheriting the C include paths). Instead, each .rc file will need to have its include paths specified directly and the include paths only apply to one particular resource script. This allows more fine-grained control and has less potentially surprising behavior (at the cost of some convenience).

Closes #19605
@andrewrk andrewrk modified the milestones: 0.14.0, 0.13.0 May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@squeek502 squeek502

Labels
bug Observed behavior contradicts documented or intended behavior regression It worked in a previous version of Zig, but stopped working.
Projects
None yet
Milestone
0.13.0
Development

Successfully merging a pull request may close this issue.

Build system: Allow specifying Win32 resource include paths using LazyPath squeek502/zig
3 participants
@andrewrk @squeek502 @Vexu