Fix incorrect c calls which crash on osx-arm64 - Githubissues

conda-forge / ipython-feedstock

A conda-smithy repository for ipython.

BSD 3-Clause "New" or "Revised" License

2 stars 34 forks source link

Fix incorrect c calls which crash on osx-arm64 #129

Closed erykoff closed 3 years ago

erykoff commented 3 years ago

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 code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[ ] Ensured the license file is being packaged.

conda-forge-linter commented 3 years ago

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) and found it was in an excellent condition.

erykoff commented 3 years ago

Test failures say "docker: failed to register layer: Error processing tar file(exit status 1): write /opt/conda/bin/msgcat: no space left on device.". Not sure whom to contact about that...

bollwyvl commented 3 years ago

Yeah, the travis issue is a known issue right now.

Top marks for getting that upstream PR in... i know its brutal, but I'm somewhat inclined to not be patching here... or limit the patch to just the new arch, even though it's in a file called osx.py that I personally am never going to open/import.

@conda-forge/ipython thoughts on us running this patch here, in advance of the 7.21 release later this month?

erykoff commented 3 years ago

I understand the reasons for waiting, and (selfishly) this isn't a big issue for me; I have the patch built locally and installed! However, there are going to be lots of people running into this, and it would be nice to get ahead of the game a bit. If you prefer, I'd be happy to put an osx restriction on the patch. And to be clear, this was always a bug, it was just lucky that this buffer overrun happened to work on x86.

bollwyvl commented 3 years ago

Sounds like a good fix, then, and yeah, it's better to be sneaky than handle more issues... toss that selector on there, and you've got my :heavy_check_mark: !

bollwyvl commented 3 years ago

Well, given that the travis issue continues, I guess we merge... this version will have no impact on those folks, unless :apple: somehow stealth releases a modern ppc box that we somehow support, just to keep us guessing...

bollwyvl commented 3 years ago

@erykoff thanks again for the quick fix here and upstream!

bollwyvl commented 3 years ago

Looks like even travis worked! Everything should be up on CDN within the hour. Thanks again!