ping @Mindavi for review too

I couldn't find anything in the repo that sets -target (as expected) which leads me to believe that its relatively safe.

@06kellyjac give this a try

This needs to point at staging instead.

Also I don't think this is a good fix for #176128, because clang might try to produce a far binary for both arches instead. What you want is logic in cc-wrapper itself to only add a -target ... if the user didn't already pass one themselves.

Also I don't think this is a good fix for #176128, because clang might try to produce a far binary for both arches instead.

I find this surprising if true, got any links to documentation? So far everything I've seen to generate fat binaries or bundles is very much opt in with either clang args (clang offload bundler) or different tools (lipo, but I think this is osx only)

What you want is logic in cc-wrapper itself to only add a -target ... if the user didn't already pass one themselves.

Setting aside the fat binaries, I think this makes sense anyway so will look into updating cc-wrapper.

Hey @Ericson2314 I've rebased on-top of staging and re-targted the PR accordingly. Also made it so -target should only show up once in the final params just once. This ends up causing a full rebuild of clang which then ripples out, this did not happen with the previous approach, just an fyi.

I feel like there was a reason for the if, elif chaining, but maybe it's just how it was written.

Could that be because we're using a very old bash on MacOS and this is used during bootstrap or something? Hopefully someone else can confirm or deny.

@happysalada do you know?

I feel like there was a reason for the if, elif chaining, but maybe it's just how it was written.

Could that be because we're using a very old bash on MacOS and this is used during bootstrap or something? Hopefully someone else can confirm or deny.

@happysalada do you know?

I thought it might have been due to sh vs bash, but I verified that dash supports case and so I'd suspect old bash would too.

The if/elif chaining makes more sense in the other params handling further down (for paths) because of the extra conditionals. The if/elif I replaced had a few of them too but I think the nested case works just fine for them. I got rid of the && $isCxx = 0 because it doesn't really matter that we're setting isCxx=1 if its already 1 anyway and that way we don't need to nest an if.

case is in posix sh (see 2.9.4 in https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09) so should be ok to use for old mac bash.

Thanks for checking! It does look nicer with case IMO, so that's good.

Could you point me to where clang sets the target? I cannot find it in a pinch.

@Ericson2314 how does this look?

I would tweak the names and make them consistent, but otherwise looks good!

I would tweak the names and make them consistent, but otherwise looks good!

Can you elaborate? I copied the naming scheme used in bintools-wrapper:
add-darwin-ldflags-before.sh -> add-clang-cflags-before.sh
add-local-ldflags-before.sh -> add-local-cflags-before.sh

Do you want me to prefix -cflags with cc?

Does this file already exist or is it missing: add-local-cflags-before.sh?

@mmlb I don't know why bintools wrapper uses two different names either. Do you, @thefloweringash?

@mmlb I don't know why bintools wrapper uses two different names either. Do you, @thefloweringash?

Are we looking at extra "ld" in "add-local-ldflags-before.sh" that's not in "add-flags.sh"? If I was the one that introduced those (and history seems to indicate it was), then it was just an oversight.

Does this file already exist or is it missing: add-local-cflags-before.sh?

Does not exist. default.nix will create it for clang, if gcc ever needs a gcc only flag at runtime we could have a add-gcc-local-cflags-before.sh.

@mmlb I don't know why bintools wrapper uses two different names either. Do you, @thefloweringash?

Are we looking at extra "ld" in "add-local-ldflags-before.sh" that's not in "add-flags.sh"? If I was the one that introduced those (and history seems to indicate it was), then it was just an oversight.

Are we talking about the reason for add-local-cflags-before.sh vs cc-cflags-before (and the equivalent ld in bintools)? If so I think it makes sense to have the local in there. add-flags doesn't source the add-cflag.sh it reads it in directly to the variable being assigned. Both uses of local need to be run so that it can interpret the ${params[@]} array.

I'm going to modify the commit so the files have cc- in the name. It feels weird compared to all the cc-cflags here.

or do you mean I should have named add-local-cc-cflags-before.sh -> local-cc-cflags-before to follow the naming scheme better?

I don't understand why the file in the Nixpkgs src has one name (with .sh) and the file "installed" as part of the built wrapper has a different name (still with .sh). I get it when we strip the .sh on install (e.g. installing something to bin/) but I don't get why for something very internal like this.

I also don't understand why anything is called "local". What does "local" mean here?

Finally, I don't like the use of "clang" because this applies equally well to any compiler that would accept -target. Just as we renamed the variables in bash to get rid of "clang" we should also rename the file to get rid of "clang".

ah gotcha. Yeah I think the idea is that there's an "interface" and clang and gcc can both implement it by having a file named local-cc-cflags-before.sh dropped into $out somewhere. I think that makes some sense. The other option would be instead of file names we'd source all files in a dir that match a pattern but that would be up to collation order and would be totally different than what is being done currently, and idk if its even warranted tbh.

I took local as an indicator that file has code to run vs just a list of values to be read, open to w/e name makes sense to show that distinction.

re clang vs more fine grained names. I can rename so its just for target, but some of the other files here are specific to the tool not to the features so I went with that too since only clang does -target and thus yagni.

Sounds like you want me to rename from clang -> target and maybe come up with something else more relevant than local. I'll do the former rn and maybe come up with a better name than local. We can discuss that separately.

OK that makes some sense we can live with this for now.

(the approval and had some old junk queued up from before accidentally)

(the approval and some old junk queued up from before acidently)

what do you mean?

John means that he had some comments in an 'open review' and they were submitted when he merged/approved. That wasn't the intention since they were stale.

Yes exactly. Thanks.

ah, gotcha.