pupnp / pupnp

libupnp: Build UPnP-compliant control points, devices, and bridges on several operating systems.
https://pupnp.github.io/pupnp
BSD 3-Clause "New" or "Revised" License
352 stars 117 forks source link

Added cmake sanitizer #314

Closed Vollstrecker closed 3 years ago

Vollstrecker commented 3 years ago

Today I cloned the sanitizer for cmake, and added all deactivated options to the test.

I wanted to split the options to one per line, but no matter what combination of "

On win is openssl atm not buildable (give me a year or two for this), and on Mac it seems that the dev-files are not installed, so this is not tested there.

whyman commented 3 years ago

I dont think -Dunspecified_server=ON and -Dblocking_tcp_connections=ON should be set

These do not enable some feature but change its behaviour, so it makes more sense to keep them as the defaults.

Vollstrecker commented 3 years ago

I usually think it's off for a reason, and I just copied the defaults from configure.ac. But if it's not an additional feature, it should maybe tested with on and off to be complete.

mrjimenez commented 3 years ago

I wanted to split the options to one per line, but no matter what combination of "" and " " with next line indended or not, it always seemed that everthing after the first line get's swallowed. I'm not even sure if your flags get really added and not just set as vars after the configure, at least I saw something similar in the win build. An yes I used "run: |"

On win is openssl atm not buildable (give me a year or two for this), and on Mac it seems that the dev-files are not installed, so this is not tested there.

So I understand that the PR is not ok to go, right? How about leaving this PR as a draft? I tend to understand that when you place a PR that passes all tests, it is because it is ok.

Vollstrecker commented 3 years ago

It is. openssl on win is not possible atm and won't be in the future, and how to get it on mac is beyond my horizon.

mrjimenez commented 3 years ago

It is. openssl on win is not possible atm and won't be in the future, and how to get it on mac is beyond my horizon.

To me it does not appear as a draft. I mean a Github draft PR. That way I won't merge it by mistake. There is an option Convert to draft for you somewhere near the top right of the page.

Screenshot_20210407_123132

In fact, I though that only the person who presented the pull request could convert it to draft, but I have just found out that I can do it too. :laughing:

mrjimenez commented 3 years ago

It is. openssl on win is not possible atm and won't be in the future, and how to get it on mac is beyond my horizon.

Unless you meant It is ok, because I understood It is a draft. :thinking: In that case, please undo what I just did and sorry for that.

Vollstrecker commented 3 years ago

Unless you meant It is ok, because I understood It is a draft

Yeah, I shortly thought about explaining what "it is" means, but I've been too far away at this point. I meant it is OK, as I can't figure out what happens on mac until I get my hands on it (except if I push workflows with hundreds of ls ) and I don't think I get openssl to build automated on win.

So testing everything on linux and everything except openssl on the others is the best we can get atm.

mrjimenez commented 3 years ago

About -Dunspecified_server=ON

#ifdef UPNP_ENABLE_UNSPECIFIED_SERVER
        snprintf(info, infoSize, "Unspecified, UPnP/1.0, Unspecified\r\n");
#else /* UPNP_ENABLE_UNSPECIFIED_SERVER */
#ifdef _WIN32
        OSVERSIONINFO versioninfo;
        versioninfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);

        if (GetVersionEx(&versioninfo) != 0)
                snprintf(info,
                        infoSize,
                        "%d.%d.%d %d/%s, UPnP/1.0, Portable SDK for UPnP "
                        "devices/" UPNP_VERSION_STRING "\r\n",
                        versioninfo.dwMajorVersion,
                        versioninfo.dwMinorVersion,
                        versioninfo.dwBuildNumber,
                        versioninfo.dwPlatformId,
                        versioninfo.szCSDVersion);
        else
                *info = '\0';
#else /* _WIN32 */

I think this should be removed and treated in a default #else if the type of system is unknown.

As for -Dblocking_tcp_connections=ON, there should be tests for both.

You know what would be ok? If there was a matrix in which we could choose the set of flags used in the build.

Lets commit that for the time. Thanks for the good work!

Vollstrecker commented 3 years ago

I think this should be removed and treated in a default #else if the type of system is unknown.

Removing the option is no problem. I just need to know when the code is ready.

If there was a matrix in which we could choose the set of flags used in the build.

Matrix should work, the question is more what combinations are needed.

mrjimenez commented 3 years ago

I think this should be removed and treated in a default #else if the type of system is unknown.

Removing the option is no problem. I just need to know when the code is ready.

Yes, I meant the code, I am just poking to see if someone complains.

If there was a matrix in which we could choose the set of flags used in the build.

Matrix should work, the question is more what combinations are needed.

Right, I'll experiment with the workflow too.