Skip to content

fix(cc): move user provided -isystem paths to front of command #24353

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

The-Briel-Deal
Copy link

@The-Briel-Deal The-Briel-Deal commented Jul 6, 2025

Problem: Currently if you pass a -isystem argument to zig cc, the include path will be searched after all of the libc, libcpp, and native system paths. However if you run the same command against clang and gcc you'll see that any user provided -isystem paths are at the very front.

Solution: Pull out user provided -isystem from mod.cc_argv and comp.global_cc_argv at the start of addCCArgs(), append these paths as -isystem args just before we append the native system paths. Finally we append the rest of the cc args in the same place.

Fixes: #24243

@alexrp
Copy link
Member

alexrp commented Jul 6, 2025

This doesn't seem quite correct; with this change, C flags on modules will no longer be able to override any default flags that we add in addCCArgs().

@andrewrk
Copy link
Member

andrewrk commented Jul 6, 2025

That's right - those are at the end on purpose so they can override everything else.

The proper fix is to adjust the parsing in the CLI to account for isystem args without piggybacking on the general last resort overriding flag set.

@The-Briel-Deal The-Briel-Deal marked this pull request as draft July 6, 2025 19:27
@The-Briel-Deal
Copy link
Author

That's right - those are at the end on purpose so they can override everything else.

The proper fix is to adjust the parsing in the CLI to account for isystem args without piggybacking on the general last resort overriding flag set.

I think I understand! I believe I saw that some flags are parsed out earlier and are pulled out of cc_argv. That makes sense and is a fair concern. Let me spend some time reworking this change and I'll mark it as ready for review again.

Thanks for the super quick feedback! I appreciate it!

Side Note: I'd really like to add a test for this as well, originally I wanted to perform a test call to zig cc to ensure that the right header is included, then when I couldn't find a good way to do that I tried just testing Compilation.addCCArgs() but it ended up being a ton of code to get a fake Compilation struct setup. How would you guys test this sort of change?

@alexrp
Copy link
Member

alexrp commented Jul 6, 2025

Putting a test somewhere in test/standalone would make sense.

@The-Briel-Deal The-Briel-Deal force-pushed the isystem_precedence branch 9 times, most recently from 3746be5 to 8979dab Compare July 19, 2025 14:38
@The-Briel-Deal The-Briel-Deal changed the title fix(cc): move user provided zig cc args to front of command fix(cc): move user provided -isystem paths to front of command Jul 19, 2025
Problem: Currently if you pass a `-isystem` argument to `zig cc`, the
include path will be searched after all of the libc, libcpp, and native
system paths. However if you run the same command against `clang` and
`gcc` you'll see that any user provided `-isystem` paths are at the very
front.

Solution: Push all user provided arguments to the arg list at the start
of `addCCArgs()`. This makes it so that any compiler arguments that rely
on the order of arguments passed will prioritize the user provided
values first.

Fixes: ziglang#24243
@The-Briel-Deal The-Briel-Deal marked this pull request as ready for review July 19, 2025 15:14
@The-Briel-Deal
Copy link
Author

The-Briel-Deal commented Jul 19, 2025

Hey @alexrp, so I fixed this by just pulling out the -isystem paths from the global and module cc_argv, then appending the paths just before native paths are appended.

Let me know what you think. Part of me was wondering if I should pull this out in buildOutputType(), but I decided against that because isystem paths should pretty much exclusively be used for C compilation I think.

Let me know if I should tweak anything else.

Also, I added a unit test against addCCArgs() in order to validate that order is correct now.

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.

[zig cc] command line -isystem paths are incorrectly searched after zig's system paths, instead of before
3 participants