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

Fix for upstream issues with compiler path #866

Merged
merged 2 commits into from Sep 23, 2023
Merged

Conversation

tcaduser
Copy link
Contributor

This is a fix for issue #860. While I agree that it should be fixed upstream, they have not done so in a year.

AndreMiras
AndreMiras previously approved these changes Sep 21, 2023
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks, easy enough hack (once you know the bug) yet effective.
Do you mind updating the docstring with the reference to the upstream bug?

kivy_ios/toolchain.py Outdated Show resolved Hide resolved
kivy_ios/toolchain.py Outdated Show resolved Hide resolved
@misl6
Copy link
Member

misl6 commented Sep 21, 2023

Hi @tcaduser and @AndreMiras, how about applying the patches from python/cpython#96399 instead?

@AndreMiras
Copy link
Member

Hi @tcaduser and @AndreMiras, how about applying the patches from python/cpython#96399 instead?

Yes I thought it would be cleaner to patch cpython, but I assumed it would be a lot harder as I didn't realized it already existed.
If patching the different versions is easy enough then yes we could go down that route

@tcaduser
Copy link
Contributor Author

Hi @misl6, @AndreMiras,

I just reviewed the upstream patch. It will still do the wrong thing for my original case:

/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmpyicccjzp

Their patch is to perform a wildcard search for icc only on the filename, and not the full path.

image

Please let me know if you still want my fix and I'll incorporate the suggested changes.

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments and thanks for reviewing the upstream patch.
Maybe you could share them that valuable feedback.
PR is good for a merge to me, but I leave @misl6 having the final word for it.

@misl6
Copy link
Member

misl6 commented Sep 23, 2023

Thanks for addressing the comments and thanks for reviewing the upstream patch.
Maybe you could share them that valuable feedback.
PR is good for a merge to me, but I leave @misl6 having the final word for it.

Ok, let's merge it while they sort it out on their side. 👍
Thank you @tcaduser and @AndreMiras.

@AndreMiras AndreMiras merged commit 829de94 into kivy:master Sep 23, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants