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

Is there 2.x backward compatibility layer? #285

Closed
pemensik opened this issue May 19, 2023 · 15 comments
Closed

Is there 2.x backward compatibility layer? #285

pemensik opened this issue May 19, 2023 · 15 comments
Assignees
Labels

Comments

@pemensik
Copy link
Contributor

I were preparing update from 2.12 to 3.0 and tried to compile flamethrower with it. It does not compile anymore because a lot of renamed classes. Is there any backward compatible layer, which would allow supporting old code without changes? Or some way, which might allow modified code to support both 3.0 and 2.12 version?

@pemensik
Copy link
Contributor Author

It seems a bad idea to me to require other software do support version 3 only or version 2 only, but without possibility to support both versions without significant effort.

@skypjack skypjack self-assigned this May 19, 2023
@skypjack
Copy link
Owner

The main problem is that v3 represents a huge rewrite of the whole library with some changes to the API too in order to drastically reduce compilation times. It's not that easy to write a migration layer unfortunately.

@pemensik
Copy link
Contributor Author

Is any notes documented, how should old code be converted to new library format?

@pemensik
Copy link
Contributor Author

I have made surprisingly lot of conversions by crude sed script:

s/uvw::TimerHandle/uvw::timer_handle/g
s/uvw::TimerEvent/uvw::timer_event/g
s/uvw::timer_handle::Time/uvw::timer_handle::time/g
s/uvw::TCPHandle/uvw::tcp_handle/g
s/uvw::UDPHandle/uvw::udp_handle/g
s/uvw::UDPDataEvent/uvw::udp_data_event/g
s/uvw::Loop/uvw::loop/g
s/uvw::loop::Time/uvw::loop::time/g
s/uvw::ErrorEvent/uvw::error_event/g
s/uvw::ConnectEvent/uvw::connect_event/g
s/uvw::CloseEvent/uvw::close_event/g
s/uvw::ShutdownEvent/uvw::shutdown_event/g
s/uvw::EndEvent/uvw::end_event/g
s/uvw::DataEvent/uvw::data_event/g
s/uvw::WriteEvent/uvw::write_event/g
s/uvw::IPv6/uvw::ipv6/g
s/uvw::IPv4/uvw::ipv4/g
s/uvw::udp_handle::Bind/uvw::udp_handle::udp_flags/
s/uvw::tcp_handle::Bind/uvw::tcp_handle::tcp_flags/

It helped in lot of cases. Remaining cases usually were using binduvw::IPv6(std::string, port) style. Is there possible way to use address in std::string and still be able specify family, whether that should use IPv4 or IPv6? Do I have to use sockaddr if I want it specified manually?

@skypjack
Copy link
Owner

I have made surprisingly lot of conversions by crude sed script

Fair enough. Makes sense.

Is there possible way to use address in std::string and still be able specify family, whether that should use IPv4 or IPv6? Do I have to use sockaddr if I want it specified manually?

Yeah, I don't remember all the internals by heart but we mimic the libuv api 90% of the time, so I'd tell you that if you can't do it there then you can't do it in uvw and vice versa.

@pemensik
Copy link
Contributor Author

I looked into util.cpp, details::ip_addr() in new version. It seems suspicious to me. Does it work correctly for IPV6 addresses? struct sockaddr_in6 has size of 28, but normal struct sockaddr has size just of 16. There is special struct sockaddr_storage, which has size of 128, that should be able to contain any address. Since the function returns just structure of size 16, does it contain full IPv6 address when used?

@pemensik
Copy link
Contributor Author

DNS software I have seen often uses union { sockaddr sa, sockaddr_in in, sockaddr_in6 in6 } to store multiple families in single compact structure. But those use just C language. Is there any C++ standard behaviour, which ensures the current construction will always work? Even for different compilers like MSCV, gcc or clang? I haven't tested it yet, but it seems to me there is no obvious guarantee that should work reliably.

@pemensik
Copy link
Contributor Author

Fair enough. Makes sense.

It seems to me backward compatible namespace uvw { typedef UDPHandle udp_handle; }; might provide backward compatibility to types, which just changed type name. That won't work for bind() calls, which have changed from template to normal function.

Yeah, I don't remember all the internals by heart but we mimic the libuv api 90% of the time, so I'd tell you that if you can't do it there then you can't do it in uvw and vice versa.

It seems to me not in this case. I think the problem is with returning sockaddr by value, not by pointer or reference. Conversion note from libuv 0.10 mentions example how to do that. Since libuv call change just the pointer, it should be safe. But because uvw returns copy of sockaddr, I think that copy would be incomplete for sockaddr_in6. Change of pointer type is safe if it points to data long enough. It seems to me this is not the case in current implementation, where it returns the reference. But since the function returns just sockaddr, I doubt that will work correctly. Returning sockaddr_storage would be safe, just initializing structure by pointer or reference as well.

It seems it also hides possible format error in address text, it initializes just zeroed structure. Which might not be the same. Which is not done by original libuv.

@pemensik
Copy link
Contributor Author

It would be nice if any header had some defines with major (and optionally minor version). That would allow to support somehow also older version from single code, with help of preprocessor's #if UVW_VERSION_MAJOR >= 3. It does not seem possible now, which makes backward compatibility almost undoable.

@skypjack
Copy link
Owner

The main problem is probably that it's not available in the older versions.
You'd have to check if it exists, then what version is it. Otherwise, it's going to fail.

@pemensik
Copy link
Contributor Author

Sure, you may define it for older versions too:

#ifndef UVW_VERSION_MAJOR
#define UVW_VERSION_MAJOR 2
#endif

Of course it would be better to have it right from the start, but adding it for next major version would be better than nothing at all. Otherwise just some autoconf hacks might be used to detect proper version.

@skypjack
Copy link
Owner

Sure, you may define it for older versions too:

Yeah, I mean, I can't really update ie version 2.0.0 and therefore I'd have to create version 2.0.1 for that. However, it doesn't really solve the problem this way. So, it seems pointless to me.

Otherwise just some autoconf hacks might be used to detect proper version.

I think one can easily get it with a cmake machinery. You've both the libuv version and the uwv version when generating the project. So, in theory, extracting the version should be possible. Have you tried it already?

@pemensik
Copy link
Contributor Author

The previous define could be used by the project using uvw, in my case flamethrower.

No, I haven't tried cmake compat yet. But to make conditional change to the code, even that cmake would have to define some variable or define, on which conditional preprocessor could choose old or the new format. Wouldn't it be easier if the uvw defined such values itself? CMake might then provide compat definition for previous versions.

@skypjack
Copy link
Owner

Yeah, it would be easier if uvw exported a version file to use in your own software. The cmake solution can be a working one for older versions though.
The problem is that I can add this number to version ie 2.12.0 but I can't change its commit. Therefore, the best that I can do is to create version 2.12.1 with this information. To me, it looks even worse then. You've the version if using 2.x.y only if y isn't 0, otherwise I'm sorry but the version isn't available. It sounds really odd, doesn't it?
So, from my point of view, the cleanest approach is to add a version file upstream and that's is. For older versions, one can use the cmake trick or any other trick because we can't really fix it without breaking something else in the meantime (like, the version commit).
Am I missing something?

@skypjack
Copy link
Owner

skypjack commented Jul 3, 2023

Closing this due to starvation.

@skypjack skypjack closed this as completed Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants