Removal of OSX CC hack (MACOSX_DEPLOYMENT_TARGET)

[daurnimator]

Note: the below is an email I sent to the luarocks list back on January 16 2015; but it got no replies.

I can't see it in the sourceforge archives: did it get stuck in some spam filter?

---

Currently luarocks uses a hack of defining CC="export MACOSX_DEPLOYMENT_TARGET=10.<version>; gcc"
Where <version> maxes out at 5.
https://github.com/keplerproject/luarocks/blob/02eff158f095ed95cf3c889fb0e9c2efd06beeb9/src/luarocks/cfg.lua#L525

There are several problems with this:

1. Defining CC as multiple commands breaks shell scripts; it should be only set to a single command to run.
   A workaround for this issue alone is to instead use: env MACOSX_DEPLOYMENT_TARGET=10.<version> gcc
2. MACOSX_DEPLOYMENT_TARGET is needlessly limited to 10.5; Some features important features are only available in later targets.
   e.g. __thread support is only available for 10.7 and above.
3. MACOSX_DEPLOYMENT_TARGET isn't required in 10.5+
   (according to Luiz, June 2012)

I propose that we drop this CC/LD hack completely, and if someone wants to compile for an old osx version (pre 2007), they can take the 5 seconds to configure their variables manually.

Is someone perhaps more versed in OSX compilation able to say if this is a good/bad idea?

[daurnimator]

Currently luarocks uses a hack of defining CC="export MACOSX_DEPLOYMENT_TARGET=10.; gcc"
Where maxes out at 5.
https://github.com/keplerproject/luarocks/blob/02eff158f095ed95cf3c889fb0e9c2efd06beeb9/src/luarocks/cfg.lua#L525

This code area got a very small change in 02e8bbd but the issue still remains.

[daurnimator]

At a minimum, can we change from using the environmental var to using the CFLAG -mmacosx-version-min=? This will solve 1. but not address 2. or 3.

[gvvaughan]

for (1), it is more idiomatic in shell to set environment variables for a single command invocation using the env command, so that assumptions that CC and LD are a single command remain correct:

CC='env MACOSX_DEPLOYMENT_TARGET=10.8 gcc'

note also that invocations of $CC can never safely split on whitespace (and indeed shell itself never splits a dollar expansion) because the user could legitimately set CC to, say 'cc -stdc99'.

[gvvaughan]

-mmacos-version-min is clang specific precludes the use of other current or future compilers on MacOS, so I recommend against that... unless it is preceded by a feature test to make sure the compiler doesn't bail out with an unsupported option error before actual use. Even then, env is faster and cleaner.

[daurnimator]

env is faster and cleaner.

I'm happy with the env solution too; they're equivalent for my uses.

Do we want to get that commited for now, and then argue over whether we need anything at all later?

[hishamhm]

I have no issues with the env approach. (I'm on the phone now though)

[gvvaughan]

regarding (3): MACOSX_DEPLOYMENT_TARGET causes the Apple compiler, even in 10.11 betas, to change conformance with SUSv3 and legacy (pre-10.5) header declarations at compile time, and also turns on weak linking to prevent generating a binary with a load-time dependency on (indirect) API symbols not yet available in the stated target release... that is, if you compile without the setting on 10.11, and someone running 10.10 installs your binary, it may not work for them even though your source code does not directly call any 10.11 only APIs.

Removing it from Luarocks entirely seems like a mistake to me, especially as it might seem to work for a large set of recent OS releases and Lua bindings; until it doesn't. 10.8 seems like a good value already as it doesn't predate __thread support, while being recent enough not to exclude any significant portion of the Mac OS/Luarocks user base (IMHO).

Something else to consider is that child processes inherit the parent's environment in POSIX, if only we had os.setenv or similar in the core, a one time os.setenv"MACOSX_DEPLOYMENT_TARGET=10.8" before forking gcc would be all that's needed :-(

[daurnimator]

that is, if you compile without the setting on 10.11, and someone running 10.10 installs your binary

I question if anyone is actually distributing binary rocks outside of windows.
For the few that may be, they can possibly turn on MACOSX_DEPLOYMENT_TARGET themselves...

On linux, I already have to take precautions if I'm going to distribute a binary (usually building on a system with a sufficiently old glibc but a new kernel..., I know people that keep ubuntu edgy vms around for this purpose...)

[gvvaughan]

I have no idea about binary rock distribution numbers. You may be right that the only people who care about this already know how to use it... but it doesn't cost anything to leave existing code in place that might help someone who doesn't know they need it, does it?

I'll bet those Linux distributors would love to have a UBUNTU_DEPLOYMENT_TARGET feature ;-) I mean REDHAT_DEPLOYMENT_TARGET, err ANDROID_.. oh well, that must be why they don't :->

[daurnimator]

it doesn't cost anything to leave existing code in place that might help someone who doesn't know they need it, does it?

The cost is not being able to use new APIs on OSX.
e.g. when 10.11 is released, I'd like to use the new connectx syscall; however if luarocks uses MACOSX_DEPLOYMENT_TARGET of anything less than 10.11, the function definition disappears.

[gvvaughan]

[edited]

  local version = os.getenv "MACOSX_DEPLOYMENT_TARGET"
  if version == nil then
    local sw_version = io.popen("sw_vers -productVersion"):read("*l")
    local versionminor = tonumber(sw_version and sw_version:match("^[^.]+%.([^.]+)")) or 3
    if versionminor >= 10 then
      version = "10.8"
    elseif versionminor >= 5 then
      version = "10.5"
    else
      version = sw_version
    end
  end
  defaults.variables.CC = "env MACOSX_DEPLOYMENT_TARGET="..version.." gcc"
  defaults.variables.LD = "env MACOSX_DEPLOYMENT_TARGET="..version.." gcc"

?

[hishamhm]

let's just change to env in LR2, but make the deployment target settable via rockspec somehow in LR3.

[daurnimator]

let's just change to env in LR2, but make the deployment target settable via rockspec somehow in LR3.

Fair enough.

[gvvaughan]

Totally fine with me too :)

[daurnimator]

Is there an LR3 TODO list we should track this on?

[hishamhm]

@daurnimator, no but I've got a nice untested patch for the luarocks-3 branch which I'd love if you gave it a spin :)
https://gist.github.com/hishamhm/ac80272e9aa102f50a29

[daurnimator]

I'd love if you gave it a spin :)

Sorry, I don't personally run a mac, I just debug other people's when they try and install my software...

Misc comments:

• is build.macosx_deployment_target the right place in the rockspec?
  • be in variables instead?
  • be in CFLAGS instead?
  • how about in supported_platforms? => ``"osx>=10.10"`
• should hoist minor and patch_variable functions
• I don't like the idea of patching CC/LD at runtime (should only be okay when building config)

[hishamhm]

be in variables instead?
be in CFLAGS instead?

These don't sound right to me, because of the time when they are evaluated.

how about in supported_platforms? => `"osx>=10.10"

This sounds like an option, but entails a lot more work (then people will feel entitled to write "windows >= vista" and expect it to work, too).

should hoist minor and patch_variable functions

Why? They're specific to this function.

I don't like the idea of patching CC/LD at runtime (should only be okay when building config)

It's the best way to do it, for it to work for every build type.

[daurnimator]

people will feel entitled to write "windows >= vista" and expect it to work, too

Actually... why shouldn't it?
Don't use "vista", but use the windows release version e.g. vista is 6.0

[hishamhm]

Actually... why shouldn't it?

Are you submitting a patch with version checks for every supported OS? ;)

Wonder what one would use for versioning Linux distros ;) (Seriously speaking now: OS version check for Linux is a terrible idea. One should keep versioning at a per library basis.)

Anyway, we're drifting here. Since your issue and Gary's seem to be fixed by the env change, I'm closing this for now.

[daurnimator]

Are you submitting a patch with version checks for every supported OS? ;)

I can if you want. Will you accept it? :)

Wonder what one would use for versioning Linux distros ;)

Kernel version...
e.g. why that might be useful: I'll probably write a kdbus library soon: it will need a kernel new enough to support it

(Seriously speaking now: OS version check for Linux is a terrible idea. One should keep versioning at a per library basis.)

Yeah, most things actually need e.g. a libc new enough.
However that is something that should be encapsulated into external_dependencies.

Anyway, we're drifting here. Since your issue and Gary's seem to be fixed by the env change, I'm closing this for now.

Fair enough to close the issue; the immediate issue of an invalid CC is resolved.
Though I'd like to track the problem of needing __thread support somewhere.....

[hishamhm]

I can if you want. Will you accept it? :)

Probably so, if checks are not too hacky! :D

[daurnimator]

I had a go at adding this tonight; but finding the right place to slot it in was tricky.

• cfg.platforms is both an array and a set, which makes it hard to add a version to
• supported_platforms seems to have a different type of blacklisting (which I don't think anything uses?), which makes the handling different from what's done in match_deps