-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Protocol: debug_assert!() Span to reflect a valid slice #6806
Conversation
That sounds like an interesting way to catch bugs here. The first look at the test failures looks like we have some issues that get caught that way. |
Yeah that's my impression too. Currently I hit a wall because I know little to nothing about the virtualenv. Alternativly we could enable RUST_BACKTRACE=1 in CI? Potentially it could be usefull - or spammy :) Perhaps both. Below theres a snippet from the CI out put. I think we have a culprint here w.r.t. alias problems. |
I think @kubouch is the expert on the Python virtualenv integration. He may be able to help you out there. |
You need to install Python and virtualenv, then create a new virtual environment, then try to source the generated activate.nu and/or deactivate.nu. I'm quite busy nowadays to look into it but you should be able to look into virtualenv docs how to do it, it's easy, it's just a few commands. The template activation scripts are here https://github.com/pypa/virtualenv/tree/main/src/virtualenv/activation/nushell but you can't source them directly, they need to be generated by virtualenv to replace the |
7a33006
to
e323f1c
Compare
@dbuch What's the status on this PR? |
Last we talked about this one, I thought we decided it was good to land. We just need to get the ci green. Please correct me if I'm wrong. |
Sorry, i've been quite busy lately. Yes I's my impression we want this. Problem is i did manage to spin up Virtual Python environment, as @kubouch said it's easy :) At it's core atleast. I sadly couldn't trigger the problem seen in CI. I'll rebase asap and se what happens. |
1acba7a
to
29fffa1
Compare
I make some progress today and caught a Backtrace locally. With some local changes to virtualenv. Next step will be to look for some additional context around the panic around |
56b9239
to
9f87007
Compare
Yippie! Everything going green! Great work @dbuch. I think we need a few reviewer looks if the fixes are straightforward or hiding a larger problem but then we are good to land this one. |
I'm sure this should recieve some more love. Thats why i didn't my self mark as ready for review - on the other hand i don't mind do it in plenum. So if anyone has something to say or want to poke at this "fix" - please go ahead :) I'll try to elaborate abit, but i need to collect some more data. I just ran out of time and frankly I won't be able to allocate much focus on matter this until the next weekend, i think. |
Hey @dbuch, If we can fix these conflicts we think we'll be able to land this. Let us know if you have time and can work on it. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also had a look through the code. Looks all good to me!
Having the more explicit Span::unknown()
in more places is great!
Also enforce this by #[non_exhaustive] span such that going forward we cannot, in debug builds (1), construct invalid spans. The motivation for this stems from nushell#6431 where I've seen crashes due to invalid slice indexing. My hope is this will mitigate such senarios 1. nushell#6431 (comment)
Thanks! |
Also enforce this by #[non_exhaustive] span such that going forward we cannot, in debug builds (1), construct invalid spans.
The motivation for this stems from #6431 where I've seen crashes due to invalid slice indexing.
My hope is this will mitigate such senarios
Description
(description of your pull request here)
Tests
Make sure you've done the following:
Make sure you've run and fixed any issues with these commands:
cargo fmt --all -- --check
to check standard code formatting (cargo fmt --all
applies these changes)cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect
to check that you're using the standard code stylecargo test --workspace --features=extra
to check that all the tests passDocumentation