-
Notifications
You must be signed in to change notification settings - Fork 59
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
Should MACOSX_DEPLOYMENT_TARGET/LC_VERSION_MIN_MACOSX be verified? #56
Comments
Yes - that would be a good idea. Maybe warn, or error if a flag is
present? I can imagine that builders will often want to be sure they
are building wheels with valid libraries...
|
Yeah, I can imagine cases where people want to ignore this, so I think a warning by default and an error if flag present makes most sense. Something like "--strict" or so. |
Do you have any interest in a PR? My teaching term is just about to start,
so my spare time tends to 0, for the next 12 weeks or so.
|
I would but it's a bit tricky as I don't have access to a Mac. I think these are the things to be done:
If I know I said that it should print a warning by default, but then it would behave differently to the existing |
I did a test on CI and it turns out that Homebrew libraries (bottles) generally don't have a deployment target set, and that means it can't be queried with otool. This is a bit unfortunate, but I guess you could still treat it as an error but only if your host system version is newer than your target version. |
Hmm - yes - that is unfortunate. I hate to be cheeky - but do you have any interest in access to a Mac? There are a couple of beefy machines you'd be welcome to login to. |
Definitely, let's exchange details via email. |
Some new discoveries... There's a PR up at pypa/wheel#314 (see also corresponding issue pypa/wheel#312) which adjusts the platform tag during wheel creation depending on the target version of all binaries in the wheel (Python extension plus other packaged dylibs) by taking the maximum target version of all. In there, I also stumbled upon this note (pypa/wheel#314 (comment)):
I verified that and indeed I see LC_BUILD_VERSION for homebrew bottles on a 10.14 host:
So that's great, as it means this can always be checked. The question now is: If the pypa/wheel PR goes in, then there isn't really a need anymore to do any checks in delocate. If people want to check for a given target version of a wheel then they can just look at the wheel filename then and check for What I don't like about the PR is that it adjusts the version silently without any diagnostic output. There will probably also be concerns about the hand-crafted mach-o parser instead of relying on an established library or tool for that. What do you think? |
Hello I`m the author of this pull request and I'm thinking about this problem with silence change of file name. The reason that I'm not put any warning is that the default python installers are for 10.6 (intel) and 10.9 (86_64). So this may produce warnings very often. I'm not sure how to handle this. I found one python lib (https://pypi.org/project/macholib/) for parse mach-O header. It is quite big and putting it in the dependency of such basic package is not good idea. I thnk also that Mach-O header format is more stable than otool output format. And this code is not limited to one platform (good for testing purpose) |
Thanks for the message.
I would certainly warn if MACOSX_DEPLOYMENT_TARGET is set, and it doesn't
match...
Why not warn if it is _not_ set? That way you can let people silence the
warnings by setting the variable to a value that is high enough. Is there
a situation where they _shouldn't_ do that?
It would be good to have a "strict" setting where the user gets an error if
they don't match. Maybe you already have that?
I agree, macholib was far too heavy for me too. I wonder whether there's
an appetite for including the little utilities in delocate into the `wheel`
public interface?
|
If MACOSX_DEPLOYMENT_TARGET is not set, then distutils automatically sets it to the version that the Python installation was built when building extension modules. If there is no mismatch between that and any extra libraries, I wouldn't output a warning. You could consider when a wheel just contains an extension module without external libraries. In that case it will always be fine and there shouldn't be a warning. So, I would warn if the version had to be bumped, and error if MACOSX_DEPLOYMENT_TARGET is set and external libs don't match. distutils doesn't allow MACOSX_DEPLOYMENT_TARGET to be lower than what Python was built with, so probably the right matching algorithm is that the target versions of libraries must be between that of Python and MACOSX_DEPLOYMENT_TARGET, both inclusive. Considering this, then also if MACOSX_DEPLOYMENT_TARGET is not set then it should error if any library has a target version below that of Python. I'm not sure to be honest why distutils has this restriction, maybe I'm missing something here. After all, Apple guarantees forward compatibility, so if I build a module targeting 10.6 but Python is 10.9, then it should still be fine, right? |
I update the pull request. I add warnings when final macosx version is higher than on begin. @matthew-brett I`m not sure what you mean by strict settings? To fail build if it Needs higher version than MACOSX_DEPLOYMENT_TARGET tells? I can try to write something like that, but I,m not sure if I'm enough experienced with python bdist process to do it properly. Maybe some suggestion? @letmaik One problem which I see (I do not have any project to test it) is when people use external tool (like cmake, make) instead of distutils. In such case there is no guarantee that project specific code is compiled against proper version of macosx. There is no problem when some of external libraries is compiled against eg. 10.6 and your project is 10.10. There is backward compatibility. Problem is when some of wheel files needs more modern version of macosx (like brew installed libraries). |
@Czaki Your new tests look great, good work. We're on the same page, everything makes sense to me. I think you should extend the warning messages further to not just tell people how to silence the warning, but also print which libraries violate the version and that they should recompile them with MACOSX_DEPLOYMENT_TARGET if they want to target a lower version. |
@letmaik I add this information to warning message. |
@Czaki - forgive my ignorance - but it is harder to raise an error than do a warning? Or is it just that it is difficult to pass the flag, to trigger the warning? |
@matthew-brett I'm not sure how exactly looks control flow in distutils process. I understand that i it should be added to So I do not know how to pass this control variable to |
@matthew-brett my pull request to wheel was merged. |
@matthew-brett @letmaik |
Well - there are now quite a few poplular wheels - like Numpy - that claim to be |
@matthew-brett @letmaik Maybe I see one big problem. My PR to |
I don't think delocate renames the platform tag so this shouldn't be an issue. @matthew-brett Is that right? |
Scenario. You build your code against macos 10.10. So every files has macos minimal version 10.10. But your code depends on external library, and you forget to compile it against old revision (or install from brew) and your system is 10.14. Then dealocate will put copy of this library inside wheel. But now the wheel need macos in version 10.14 to run and platform tag suggest 10.10. Is this working in this way? or I'm wrong? |
Yes, that's right. Delocate does not change the wheel name, nor does it check whether the vendored libraries are compatible with the stated wheel tag. |
So my PR to wheel still not solve the issue from which this talk start. |
If macholib is doing something complicated to implement, it's not a big deal to depend on it - but we've avoided macholib so far because it was easy enough to analyze output from command line commands such as See for example: https://github.com/matthew-brett/delocate/blob/master/delocate/tools.py#L199 |
Ok. If dealocate base on parsing otool then any change of output format will produce problem globally. |
OK, I understand the issue now. This is actually a problem. |
As you see in my PR code to wheel I create code to check minimal version of macos. It will be easy to extend this code to check also if I'm not so familiar with macholib to tali how many lines of code its needs to get same information. I do not know dealocate code enough to think how many of code is needed to replace parsing otool output. I can try, but on begin there is need decision if use macholib or custom code (the code using otool to determine proper tag could be also put in another PR) |
I really don't mind if you'd prefer to use macholib. Why not do a PR using macholib and we can see if it's worth re-implementing the check with otool? |
I'm slightly confused, are we talking about a PR towards |
I think @Czaki is proposing to do a PR to delocate, and he is considering using As I understand it, current |
@Czaki - I guess you're trying to avoid copying your detection code out of the |
@matthew-brett Yes. I think about PR to dealocate. I'm ok wit copy detection code from wheel package. Or I can try write code which will import it from wheel. This which is not implemented in wheel is checking of architecture. Both, my code and macholib parse macho header, so if them give different answer, then one of them need to have bug. |
I wouldn't import from wheel, they don't like people importing their code, because it is more effort for them to maintain. |
@matthew-brett I strongly prefer to fix pep8 warnings. It is ok for you for change formating? If it should be put in separated PR or could be part of pr with checking system version? |
Could you make a separate PR with the pep8 changes? I promise to merge quickly. |
I created PR #60 |
@matthew-brett Coul I ask you for review template of interface for proposed changes? |
I have upgrade draft. Czaki@9a90044 I have one conception problem. When I read pep 427 it told that wheel may contain only one platform tag. Base on this I create current code that will calculate proper name (when filled But if my assumption is true? Or there are wheels that can have multiple platform tag? |
@matthew-brett in file
|
@matthew-brett @letmaik Did you have access to machines with old MacOS? I have problemwith generate sample data on MacOS 10.14. During try to generate code I got such messages:
I decide to firs create whole structure and test data and then implement. All base changes I put in https://github.com/Czaki/delocate/tree/weel_name_fix_base and creation test data here: https://github.com/Czaki/delocate/blob/weel_name_fix_base/delocate/tests/data_platform_tag/make_libs.sh If I good remember on MacOs 10.13 there is possible to compile 32 bits lib. |
Not directly, but Travis CI has many versions: https://docs.travis-ci.com/user/reference/osx/#macos-version Theoretically you could create a dummy repo with Travis CI enabled which does what you need to do and then upload the files somewhere. It may take some time to get it all working. |
@letmaik @matthew-brett any suggestions? |
Sorry that this was left alone for so long @Czaki, can you tell me the current status of this issue and your PR's #61 #68? I should be able to review your PR's whenever they're ready, and answer any other questions you have.
If your PR's are duplicating the functionality of |
When building wheels for macOS target version 10.9 (for example), then delocate will happily pull in dependencies that have a higher target version. In most cases this won't cause issues, but I think there should at least be a warning.
From https://stackoverflow.com/a/17462763, this is how to show the target version from a lib:
$ otool -l foo.dylib | grep -A 3 LC_VERSION_MIN_MACOSX cmd LC_VERSION_MIN_MACOSX cmdsize 16 version 10.8 sdk 10.8
This will typically happen when using homebrew to get dependencies. The downloaded bottles will have the same target version as the host OS. Unfortunately, brew can't be forced to build packages from source with a custom target version: https://discourse.brew.sh/t/it-is-possible-to-build-packages-that-are-compatible-with-older-macos-versions/4421. The only correct solution then would be either to build wheels on an old macOS version that becomes the target version, or to not use homebrew and build dependencies manually while setting
MACOSX_DEPLOYMENT_TARGET
accordingly.The text was updated successfully, but these errors were encountered: