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

Add excluded_search_path_prefixes setting - improves perf in WSL #1165

Merged
merged 3 commits into from
Apr 21, 2021

Conversation

stuartleeks
Copy link
Contributor

Allows filtering the paths used to search for commands
Can be useful to filter out /mnt/ in WSL for performance

Running in WSL with default config:

DEBUG: Total took: 0:00:10.213128

Running in WSL with excluded_search_path_prefixes = ['/mnt/'] in settings.py:

DEBUG: Total took: 0:00:00.617300

This provides an approach to solving #1036 without having to remove Windows paths from PATH (which breaks various WSL use-cases)

@stuartleeks stuartleeks changed the title Add excluded_search_path_prefixes setting Add excluded_search_path_prefixes setting - improves perf in WSL Feb 4, 2021
@stuartleeks
Copy link
Contributor Author

Just checking back to see whether there is anything I can do to help make this easier to review? Thanks!

@swirle13
Copy link

What would it take to get this pulled in?

Copy link
Collaborator

@scorphus scorphus 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 the valuable contribution and for a very complete patch, @stuartleeks! Much appreciated! 👍

Please, consider my suggestions below.

@stuartleeks
Copy link
Contributor Author

stuartleeks commented Apr 17, 2021

Thanks for the review @scorphus! I've included your suggestions and rebased on latest master

However, I notice that the automated checks are failing on python 2.7 at what looks to be the install stage. I'm not sure how the changes made would have caused this and have just tested installing on 2.7.17 locally and it seemed to work fine (unit and functional tests pass locally on 2.7.17, too). If you have any pointers for investigating further that would be greatly appreciated!

@scorphus scorphus added this to the 3.31 milestone Apr 17, 2021
@scorphus
Copy link
Collaborator

Glad you're still on it, @stuartleeks. Sorry for the delay.

I wonder why pip is installing decorator 5.x for Python 2.7. It should install 4.4.2 instead. Anyway, I didn't investigate too much into that issue. Instead came up with #1187 that forces decorator<5 for Python <= 2.7. Once 3.31 is released, this and other makeshifts will be removed.

I'll get #1187 merged soon. Then you're free to rebase on top of master again. Thanks in advance!

@stuartleeks
Copy link
Contributor Author

Ok, cool. I'll rebase again once #1187 is merged and hopefully this will be ready to go 😁
Thanks!

stuartleeks and others added 3 commits April 17, 2021 21:17
Allows filtering the paths used to search for commands
Can be useful to filter out /mnt/ in WSL for performance
@stuartleeks
Copy link
Contributor Author

Rebased now that #1187 is merged and the tests are now passing - thanks @scorphus !

@swirle13
Copy link

can't wait to update with these changes implemented with 3.31. I can't believe me commenting here kicked off the attention this needed to finally get merged and now I don't have to worry about manually building these changes into every new release haha

@scorphus
Copy link
Collaborator

Thanks for the update, @stuartleeks! I'll be merging this shortly!

@mjohnson159 Thanks for commenting and bringing it to my attention. We really need to get 3.31 out, right!? It's been a long time since the last release. Would you be willing to help me confirming fixes for some other Windows-related issues? That would be super helpful!

@scorphus scorphus merged commit 6da0bc5 into nvbn:master Apr 21, 2021
@scorphus
Copy link
Collaborator

Thanks for contributing, @stuartleeks! ❤️

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.

3 participants