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

Forward port arm64 patch from 14.x to 15.x #314

Merged
merged 14 commits into from
Feb 16, 2025

Conversation

kkraus14
Copy link

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Fixes #313

@kkraus14
Copy link
Author

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Feb 14, 2025

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • ❌ You are setting MACOSX_SDK_VERSION below c_stdlib_version, in conda_build_config.yaml which is not possible! Please ensure MACOSX_SDK_VERSION is at least c_stdlib_version (you can leave it out if it is equal).
    If you are not setting c_stdlib_version yourself, this means you are requesting a version below the current global baseline in conda-forge (10.13). If this is the intention, you also need to override c_stdlib_version and MACOSX_DEPLOYMENT_TARGET locally.

For recipe/meta.yaml:

  • ℹ️ noarch: python recipes should usually follow the syntax in our documentation for specifying the Python version.
    • For the run section of <output 4 output, you should usually use python >={{ python_min }} for the python entry.
    • If the package requires a newer Python version than the currently supported minimum version on conda-forge, you can override the python_min variable by adding a Jinja2 set statement at the top of your recipe (or using an equivalent context variable for v1 recipes).
  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13337499748. Examine the logs at this URL for more detail.

@jakirkham
Copy link
Member

Thanks Keith! 🙏

There are some linter fixes in PR: #271

Think we can backport those. Note these affect how GLIBC is configured. So we would want those even if there was no lint error

@kkraus14
Copy link
Author

@conda-forge-admin, please rerender

Copy link

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

This looks good to me - I notice there's a bit of an offset in RuntimeDyldELF.cpp due to some other unrelated changes between 14 and 15, but the patching still succeeds:

checking file llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
Hunk #1 succeeded at 2352 (offset 7 lines).

Copy link

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

I approve again after the recently-pushed changes :-)

@jakirkham
Copy link
Member

jakirkham commented Feb 14, 2025

Thanks Keith and Graham! 🙏

Looks like we need this one too: #310

Would also add setuptools after pip

@h-vetinari
Copy link
Member

You'll also need at least 9ec8468, 41c372e, 1809439 and potentially faaa9ad

@kkraus14
Copy link
Author

@conda-forge-admin, please rerender

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

To make the linter happy, please remove

MACOSX_SDK_VERSION: # [osx and x86_64]
- 10.12 # [osx and x86_64]

kkraus14 and others added 2 commits February 14, 2025 16:20
Co-authored-by: h-vetinari <h.vetinari@gmx.com>
@kkraus14
Copy link
Author

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Feb 14, 2025

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13348585909. Examine the logs at this URL for more detail.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13337724912. Examine the logs at this URL for more detail.

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Since this branch is really old, I cannot rule out that further fixes may be needed, but in any case, the changes here LGTM.

@h-vetinari h-vetinari added the automerge Merge the PR when CI passes label Feb 14, 2025
@kkraus14
Copy link
Author

Thanks for your help @h-vetinari!

@h-vetinari
Copy link
Member

No problem, thanks for the PR!

Though I don't know which version llvmlite will move to next (thoughts @gmarkall?), we might want to fix this on the 16.x branch as well. AFAICT from looking at llvm/llvm-project#61402, the backport to 16.x never happened: https://github.com/llvm/llvm-project/blame/release/16.x/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp#L2408

At least it should be fixed as of v17.

@gmarkall
Copy link

gmarkall commented Feb 14, 2025

we might want to fix this on the 16.x branch as well.

Yes, I'd agree it should be. I looked today at 16.0.6 and it wasn't applied, but it's present on 17.0.6. Even if Numba is not using it and may skipping it, any other user of LLVM on AArch64 can still be affected by the issue and suffer from segfaults.

LLVM 15's lit tries to `import pipes`, which was removed in Python 3.11
@h-vetinari h-vetinari added automerge Merge the PR when CI passes and removed automerge Merge the PR when CI passes labels Feb 15, 2025
@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Feb 15, 2025

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@kkraus14
Copy link
Author

2025-02-15T03:18:36.0410070Z cd $SRC_DIR/build/test && /Library/Frameworks/Python.framework/Versions/Current/bin/python3.13 $SRC_DIR/build/./bin/llvm-lit -sv $SRC_DIR/build/test

Looks like the osx build is using a system python instead of the environment python and getting a newer version than intended so pipes is no longer available.

@h-vetinari
Copy link
Member

You should be able to change

python ../../build/bin/llvm-lit -vv Transforms ExecutionEngine Analysis CodeGen/X86

to call the right python, e.g. $PREFIX/bin/python.

@h-vetinari h-vetinari removed the automerge Merge the PR when CI passes label Feb 15, 2025
@h-vetinari h-vetinari added the automerge Merge the PR when CI passes label Feb 15, 2025
@h-vetinari h-vetinari added automerge Merge the PR when CI passes and removed automerge Merge the PR when CI passes labels Feb 15, 2025
@conda-forge-admin conda-forge-admin merged commit 481dc05 into conda-forge:15.x Feb 16, 2025
9 checks passed
@h-vetinari
Copy link
Member

we might want to fix this on the 16.x branch as well.

Yes, I'd agree it should be.

Done in #315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants