Use a more careful check in remove_avoided_requirements, and add PyWavelets recipe for testing
#60
Conversation
hoodmane
force-pushed
the
less-sloppy-remove-avoided-reqs
branch
from
a612e38 to
953269d
Compare
See upstream PR: pyodide/pyodide-build#60 Resolves pyodide#5187. In that issue, the build requires a package called `cmake-build-extension` but because we have `cmake` in the list of requirements to avoid we drop it and it breaks the build. This logic was introduced in pyodide#2272 and did not come up in the review. Surprisingly this is the first problem it has caused. Let's see if this change breaks anything in Pyodide CI.
Resolves pyodide/pyodide#5187. In that issue, the build requires a package called `cmake-build-extension` but because we have `cmake` in the list of requirements to avoid we drop it and it breaks the build. This logic was introduced in pyodide/pyodide#2272 and did not come up in the review. Surprisingly this is the first problem it has caused. Let's see if this change breaks anything in Pyodide CI.
hoodmane
force-pushed
the
less-sloppy-remove-avoided-reqs
branch
from
953269d to
8ea7f43
Compare
See upstream PR: pyodide/pyodide-build#60 Resolves pyodide#5187. In that issue, the build requires a package called `cmake-build-extension` but because we have `cmake` in the list of requirements to avoid we drop it and it breaks the build. This logic was introduced in pyodide#2272 and did not come up in the review. Surprisingly this is the first problem it has caused. Let's see if this change breaks anything in Pyodide CI.
|
As CMake is no longer an avoided requirement post #141, I think packages that use |
|
Actually, if I think this change would make sense if we were to add another requirement we need to avoid which causes troubles with building, and in principle it's a good change as we match the name of the requirement exactly. At the same time, things are fixed now and I'm inclined to say "don't fix what isn't broken"... Leaving it up to you to decide, @hoodmane :) |
|
I think this is still useful, but we need to fix the problem with |
|
Could we explicitly add |
Yes, if it works, then I am okay with adding it as a workaround. I'll work on #68 later to handle it in a better way. |
Co-Authored-By: Hood Chatham <[email protected]> Co-Authored-By: Gyeongjae Choi <[email protected]>
|
Is there an offending package that requires |
Not in our integration test I think. |
Could you please clarify? As in, do you mean we don't t have a package like this, or that we don't want to add an integration test for such a package? I think this should be good to go, but CI in the Pyodide repo has been failing since quite some time now, which makes things difficult to verify. |
"do you mean we don't t have a package like this" this one. I think it is good idea to add a package that depends on NumPy. |
|
Okay, I've added PyWavelets, as it depends on NumPy and Cython at build time. It has a small codebase and is fairly fast to build as a result. However, |
Makes sense. I think it is okay for now. Maybe we can make |
That sounds like a good idea. Do you think we should leave that for a follow-up PR? If yes, @hoodmane, please feel free to merge. :) |
There was a problem hiding this comment.
Okay, I think we should merge this now, as:
- it fixes the original error, and
- it adds PyWavelets as a recipe that depends on NumPy, and
- adds oldest-supported-numpy, and
- my PR #187 currently builds on top of this PR.
Thanks, @hoodmane and @ryanking13!
agriyakhetarpal
changed the title
remove_avoided_requirementsremove_avoided_requirements, and add PyWavelets recipe for testing
3 participants
Resolves pyodide/pyodide#5187. In that issue, the build requires a package called
cmake-build-extensionbut because we havecmakein the list of requirements to avoid we drop it and it breaks the build.This logic was introduced in pyodide/pyodide#2272 and did not come up in the review. Surprisingly this is the first problem it has caused. Let's see if this change breaks anything in Pyodide CI.