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

don't try to run lief on static libraries #4808

Closed
wants to merge 6 commits into from

Conversation

h-vetinari
Copy link
Contributor

Fixes #4787

@conda-bot
Copy link
Contributor

We require contributors to sign our Contributor License Agreement and we don't have one on file for @h-vetinari.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature.

@h-vetinari
Copy link
Contributor Author

Just signed the CLA, let me know if there's anything else left to do to unblock this aspect.

@1player
Copy link

1player commented Mar 17, 2023

I tried this patch on my custom build of LLVM/compiler-rt on macOS and conda build just hung on a loop when it reached the packaging phase. Have you confirmed that it works for you?

@h-vetinari
Copy link
Contributor Author

Have you confirmed that it works for you?

No, so far I just followed the suggestion given in the issue (+ some educated guessing, e.g. what's going to be in files). It's not clear to me how to write tests for this, but I also don't know the conda-build code base.

On the other hand, I cannot see how this would hang more or differently with the patch than without. There's no recursion that I can see (_collect_needed_dsos is only called once in the whole codebase), and it just means we do strictly less work (by continue-ing a bunch of items in a loop; an operation that's also done in another case just below).

@1player
Copy link

1player commented Mar 17, 2023

Yes, you are right once again :) It just takes an enormous amount of time to do the post-compilation checks on macOS. Takes literally as long as the entire compilation of LLVM, but seems to complete eventually, so looks like this fix is the way to go. Thanks.

@1player
Copy link

1player commented Mar 17, 2023

I'm not sure this is working correctly... There are some .a files generated by LLVM's compiler-rt that are not copied to the final tarball, and during compilation I see a ton of "is not an ELF" and "Relocation #1 of __text seems corrupted" messages.

I also see a "WARNING :: Failed to get_static_lib_exports" on an archive file that was previously breaking conda-build (lib/clang/../lib/libclang_rt.builtins_i386_osx.a), and with this PR this file is skipped altogether and not included in the package tree, which is incorrect behaviour.

@h-vetinari
Copy link
Contributor Author

I'm not sure this is working correctly... There are some .a files generated by LLVM's compiler-rt that are not copied to the final tarball, and during compilation I see a ton of "is not an ELF" and "Relocation #1 of __text seems corrupted" messages.

What you're seeing is normal. Please have a look at the logs of the compiler-rt-feedstock before you conclude something must be amiss. The changes here are intentionally minimal, and will not affect which static libraries are packaged or not (that's the job of the recipe build scripts).

@1player
Copy link

1player commented Mar 21, 2023

Be that as it may, there might have been a user error the first time, but I tried again this patch on a fresh install of macOS and miniconda3, and I got this error during packaging:

Traceback (most recent call last):
  File "/Users/user/miniconda3/bin/conda-build", line 11, in <module>
    sys.exit(main())
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/cli/main_build.py", line 495, in main
    execute(sys.argv[1:])
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/cli/main_build.py", line 475, in execute
    outputs = api.build(
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/api.py", line 180, in build
    return build_tree(
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/build.py", line 3097, in build_tree
    packages_from_this = build(metadata, stats,
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/build.py", line 2369, in build
    newly_built_packages = bundlers[pkg_type](output_d, m, env, stats)
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/build.py", line 1658, in bundle_conda
    files = post_process_files(metadata, initial_files)
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/build.py", line 1504, in post_process_files
    post_build(m, new_files, build_python=python)
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/post.py", line 1321, in post_build
    check_overlinking(m, files, host_prefix)
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/post.py", line 1227, in check_overlinking
    return check_overlinking_impl(m.get_value('package/name'),
  File "/Users/user/miniconda3/lib/python3.10/site-packages/conda_build/post.py", line 1146, in check_overlinking_impl
    needed = needed_dsos_for_file[f]
KeyError: 'lib/clang/14.0.6/lib/darwin/libclang_rt.osx.a'

@1player
Copy link

1player commented Mar 21, 2023

I was finally able to make this work by reverting this change, but instead updating the codefile_class function in

def codefile_class(filename, skip_symlinks=False):
to return None if the file ends in .a.

Feel free to incorporate it into your PR if you feel it makes sense.

@beeankha
Copy link
Contributor

@conda-bot check

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 23, 2023
@jezdez
Copy link
Member

jezdez commented Mar 24, 2023

This requires a news file and if possible a test case

Suggested-By: Isuru Fernando <isuruf@gmail.com>
@h-vetinari
Copy link
Contributor Author

Sorry this fell off the radar

@1player: Feel free to incorporate it into your PR if you feel it makes sense.

I did that, but I'm flying blind here - the function is not documented and I don't know the internals here. But I'm glad you tested it, thanks a lot!

@jezdez: This requires a news file and if possible a test case

Added a news file; not sure how a test case would look like, short of building one of the failing feedstocks.

Suggested-By: Stéphane Travostino <sph@combo.cc>
@h-vetinari
Copy link
Contributor Author

Gentle ping @beeankha @jezdez on how to proceed here

news/4787-fix-lief Outdated Show resolved Hide resolved
@jezdez
Copy link
Member

jezdez commented May 25, 2023

Closing in favor of #4900

@jezdez jezdez closed this May 25, 2023
@h-vetinari h-vetinari deleted the lief branch May 25, 2023 20:12
@github-actions github-actions bot added the locked [bot] locked due to inactivity label May 25, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024
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
Archived in project
Development

Successfully merging this pull request may close these issues.

Unexpected binary is None in liefldd.py
5 participants