modal-client icon indicating copy to clipboard operation
modal-client copied to clipboard

Re-implements network check with vendored `psutil`

Open luiscape opened this issue 1 year ago • 8 comments

Adds the same logic from this PR but with vendored psutil so we don't have to install an additional package.

luiscape avatar Feb 15 '24 19:02 luiscape

Can you include the copyright notice?

https://github.com/giampaolo/psutil?tab=BSD-3-Clause-1-ov-file

ekzhang avatar Feb 21 '24 19:02 ekzhang

Also can we prefix the location with an underscore (e.g. _vendor so that it doesn't get confused for public API?

mwaskom avatar Feb 21 '24 20:02 mwaskom

What is our approach to tests going to be with vendored code? Are we just going to assume that our existing unit tests cover all the behaviors that we need?

mwaskom avatar Feb 26 '24 16:02 mwaskom

It also feels like we want some way of tracking what version the vendored code is sourced from. How will we know when we need to update it, e.g. if there is an important bug in the source repository?

mwaskom avatar Feb 26 '24 16:02 mwaskom

While these are reasonable comments, I don't think they are relevant to this PR, as the use of psutil here is just a particular function that just reads and parses /proc for diagnostic reasons. We can revisit it if we need more functionality.

Other dependencies will need version tracking and so on, but this one is very easy to strip out.

ekzhang avatar Feb 26 '24 18:02 ekzhang

Now is probably the only time we will know what version this corresponds to though :)

mwaskom avatar Feb 26 '24 18:02 mwaskom

It shouldn't matter what version it corresponds to — barring any changes to the POSIX API, it should not need updates.

ekzhang avatar Feb 26 '24 18:02 ekzhang

Are we just going to assume that our existing unit tests cover all the behaviors that we need?

This should be the case, yes. The tests I added test our interface and assumes the vendored code works as it should. That's is acceptable imo.

It also feels like we want some way of tracking what version the vendored code is sourced from.

I'll add a note to comments if only to point to the right license / code version. I agree with Eric that anything more is overkill.

luiscape avatar Feb 26 '24 19:02 luiscape