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.

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

Ignore patching Mach-O files if not running on macOS #3912

Merged
merged 2 commits into from
Mar 27, 2020

Conversation

chrisburr
Copy link
Contributor

@chrisburr chrisburr commented Mar 25, 2020

As found in conda-forge/cmake-feedstock#115 (comment) the Linux CMake build produces a Mach-O binary which then triggers this assertion and causes the build to fail.

This is a regression in 3.18 as this function used to never be called on other platforms: https://github.com/conda/conda-build/pull/3808/files#diff-60400bbbdb51124a0a8a06ca17ecec5cL1105

cc @jakirkham

@cla-bot cla-bot bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 25, 2020
@mingwandroid
Copy link
Contributor

Would removing the assert and building install_name_tool in Linux not be a better approach here?

@jakirkham
Copy link
Member

This seems pretty clean and simple to me. Thanks @chrisburr 😀

Why do we need install_name_tool on Linux?

@mingwandroid
Copy link
Contributor

mingwandroid commented Mar 25, 2020

Because as seems to be the case here we are building broken macOS software on Linux.

@chrisburr
Copy link
Contributor Author

Would removing the assert and building install_name_tool in Linux not be a better approach here?

Definitely and conda-forge already has it in cctools, though the binaries are prefixed like x86_64-apple-darwin13.4.0-otool and x86_64-apple-darwin13.4.0-install_name_tool. I'm not sure how best to handle the prefix? Also what if a Mach-O binary appears during a build on Windows?

As it stands the import is commented out so other changes will also be needed:

if sys.platform == 'darwin':
from conda_build.os_utils import macho

Because as seems to be the case here we are building broken macOS software on Linux.

In the case of CMake it's actually okay as the binary in question doesn't need to be modified to make it relocatable so we're not actually building broken software. That said, there is the potential to but I think it's still better than the current situation where it's impossible to build CMake.

@mingwandroid
Copy link
Contributor

Thanks!

@jakirkham
Copy link
Member

Thanks Chris and Ray! 😄

Copy link

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to thank @chrisburr for the fix as I was struggling with that exact issue today a lot (e.g., in this build).

assert sys.platform == 'darwin'
if sys.platform != 'darwin':
log.warn("Found Mach-O file but patching is only supported on macOS, skipping: %s", path)
return
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a standard style (2 blank space indents) here in conda-build? I saw 4 space indents in many other codes of the code base.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be 4 spaces. Can you please send a PR to fix it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #3925 adjusts the indentation and fixes another bug introduced here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@chrisburr chrisburr deleted the patch-3 branch April 2, 2020 04:44
@github-actions
Copy link

Hi there, thank you for your contribution!

This pull request has been automatically locked because it has not had recent activity after being closed.

Please open a new issue or pull request if needed.

Thanks!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Mar 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants