update: numpy, pandas, sdl2
#3164
Conversation
T-Dynamos
force-pushed
the
numpy-update
branch
4 times, most recently
from
40f08b6 to
a09a2b3
Compare
T-Dynamos
force-pushed
the
numpy-update
branch
3 times, most recently
from
1cd30ca to
edea820
Compare
|
Ready for review. Hope it can be merged soon! (One action failed, probably due to an internet connection error) |
|
@AndreMiras any updates on this? |
There was a problem hiding this comment.
Yeah thanks for addressing the site package exclude thingy in a different way, at least now it doesn't impact all the other recipes.
However, generally speaking I'm cautious with changes in the core code that introduce a bit more complexity specially if it's for some very rare cases, I hope you understand.
While the changes in download_if_necessary() are easy to grasp and could easily help for more than a couple of recipes, I still want to believe we could try a different approach for the site package exclude dir exception case.
Maybe addressing it from within the Numpy recipe wouldn't be the sexiest, but at least it would be more contained.
Would you mind giving this specific part an extra thought otherwise I can give it a try when I get a chance.
This might as well not be easy at all or would in fact introduce much more complexity and completely off balance the benefits. I haven't explored, but it might be worth thinking about it and giving it a try.
Edit: my naive thought on that one is to Recipe.get_recipe('python3', self.ctx) from numpy prebuild_arch() to then mutate site_packages_dir_blacklist there. It's definitely hacky, but much more contained.
Also if @misl6 thinks otherwise, I totally trust him to make the right call, he's much more active and knowledgeable that anyone else regarding this code base
| @@ -48,7 +48,7 @@ def temp_directory(): | |||
| temp_dir, Err_Fore.RESET))) | |||
|
|
|||
|
|
|||
| def walk_valid_filens(base_dir, invalid_dir_names, invalid_file_patterns): | |||
| def walk_valid_filens(base_dir, invalid_dir_names, invalid_file_patterns, excluded_dir_exceptions=[]): | |||
There was a problem hiding this comment.
I know this looks convenient, but could you update to excluded_dir_exceptions=None instead. And then in the function body you can excluded_dir_exceptions = [] if excluded_dir_exceptions is None else excluded_dir_exceptions.
There's a gotcha with mutable default arguments in Python that could lead to bugs painful to track.
https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
I know we don't currently mutate it in your case, but I think it's a good rule of thumb to generally avoid using a mutable in default arguments at all so nothing weird happens if someone thinks about mutating this list later.
But more here than for the min_ndk_api_support change I would feel more comfortable to have this handled outside of the core code if possible. It's a very specific case from numpy, don't you think we could find a way to handle it from numpy directly in an acceptable way?
Closes #3163
Split #3136