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

Port aiosip to use the new ursine library for SIP #116

Open
wants to merge 1 commit into
base: ursine
Choose a base branch
from

Conversation

vodik
Copy link
Contributor

@vodik vodik commented Apr 27, 2018

So, @dtkerr did the hard work, and we ported aiosip to using them.

Few things to look at before merge them:

  1. I think there are a few cases where we don't generate completely correct SIP uris, so I suspect merging this might actually be a regression until we sort this out. I'm looking into this currently
  2. I've noticed that, while all the tests pass, there seems to be some retransmission issues when watching it from Wireshark. But I think this is a problem on mainline. Planning to dig into it this weekend.

But that said, this helps cleanup the API.

@vodik
Copy link
Contributor Author

vodik commented Apr 27, 2018

And I forgot to fix up the examples as well...

@vodik
Copy link
Contributor Author

vodik commented Apr 27, 2018

And lastly, I didn't put too much effort in the get_details because its going to change with #112, which can change it to frozendict((call_id, from_details.tag, to_details.tag))

@ovv
Copy link
Contributor

ovv commented May 10, 2018

Do you guys have any plans to support other things than URI in ursine ? I think we already had a bit of a discussion about removing part of the aiosip parsing into an external library. We could do it step by step with ursine as some part of the API could afford a rework.

And I'm fairly confident it would make the aiosip code much more readable

@vodik
Copy link
Contributor Author

vodik commented May 10, 2018

@dtkerr has SIP Header and URI support in there at the moment. There are rough plans to add support for Via headers as well.

Which is something I've been meaning to bring up. @dtkerr been itching to move it to some sort of parser generator library (not sure if he has one in mind), but I've been resistent for now as I wanted to keep the dependency footprint small and not make that decisoin alone.

Completely seperating the SIP message parser would probably be ideal. I'm a big fan of that kind of seperation in general and opens the door to optimizing it (for example, replace it with cython)

@ovv
Copy link
Contributor

ovv commented May 11, 2018

Completely seperating the SIP message parser would probably be ideal. I'm a big fan of that kind of seperation in general and opens the door to optimizing it (for example, replace it with cython)

We have the same goal 👍

There are rough plans to add support for Via headers as well.

Awesome 🎉 !

move it to some sort of parser generator library

To be honest I'm not sure what that would exactly requires

Moving in that direction it might be time to create that org we talked about so those projects live together.

@ludovic-gasc
Copy link
Contributor

Hi @vodik and @dtkerr,

Thanks a lot for your awesome work, I'm OK to replace the internal URI parser of aiosip by ursine.

Completely seperating the SIP message parser would probably be ideal. I'm a big fan of that kind of seperation in general and opens the door to optimizing it (for example, replace it with cython)

It looks like the sans I/O approach.
I'm agree with this approach, while we try to keep simple the maintenance ;-)

BTW, when I have created aiosip, I have taken some inspiration from @theintencity with this document and p2p-sip.

Moving in that direction it might be time to create that org we talked about so those projects live together.

Yes, definitely, and I still need to answer to #98 and #99 ;-)
I will answer you between now and next week.

Have a nice week-end.

@idletea
Copy link
Contributor

idletea commented May 11, 2018

My (potential) parser-generator plans would just be that it's probably easier to cover all parts of the URI grammar with a language meant to express a grammar, but if I ever jump for doing that I'd try to choose a parser generator that outputs code. (IE: no dependencies for the user, only for build-time).

For the time being, though, the pure python stuff in ursine looks like it functions well enough.

This PR got a bit stale pretty quickly, though, because I did some non-trivial overhauling of ursine and I'm working now on bringing this PR up to to date again.

@vodik
Copy link
Contributor Author

vodik commented May 11, 2018

Funny enough, I think that's the same guy my friend @moises-silva pointed me to - just rather to his blog posts.

I'm digging into it, its giving me ideas for #124

Copy link

@zw5 zw5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thik this is definetely healthy

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.

5 participants