Feedback requested on port of libs3 to MSVC

[ned14]

Ruslan added libs3 to cmake hunter last year, but it only compiles on POSIX. At work we badly needed libs3 as the AWS C++ SDK is shocking, so I went ahead and ported it to MSVC, and you can find that PR at hunter-packages/libs3#1.

Now, these changes are a bit severe, so I've opened this issue to discuss them:

• All VLAs have been replaced with vla<T>, as MSVC doesn't support VLAs.
• Due to the use of C++, half the C source files were ported to C++. I retained the C linkage throughout however, so it'll be ABI compatible to before.
• I disabled all the functions which use select(), as they are unfit for modern programs with large fd counts, and they would also drag in an otherwise unnecessary <winsock2.h> include to the primary interface header. The internal libs3 functions which need to go wait on fds to signal I rewrote to use the non-select() libcurl API, and those work fine.

We've rolled this library into production this morning, so we believe it is working well.

Comments? Thoughts? Is it okay if we merge this to cpp-pm and publish it?

[NeroBurner]

Do you think you can bring those patches upstream. The less changes we have to carry in hunter the better

[ned14]

The last commit to https://github.com/bji/libs3 was on 9th April 2019. I've opened a PR for my changes at bji/libs3#100, but even if the maintainer is still active (all the unanswered PRs and issues would suggest not), his is a C not C++ library. It wouldn't be especially hard to rewrite my changes into C, but that is more effort than I needed to do, especially when its maintainer appears to have gone inactive.

[rbsheth]

Hmm, I'm okay with this as long as we separate the CMake changes into a separate commit. @ned14 Can you make your PR against https://github.com/cpp-pm/libs3 and not hunter-packages?

[ned14]

I'm sorry for the wrong destination of PR. A mistake. Here it is corrected: cpp-pm/libs3#1

Re: separating the CMake changes into a separate commit, I deliberately left Ruslan's commits alone and didn't combine commits nor rebase. It's tricky to know what is best to do here, either we could rebase the cmake changes so they come after the other changes to the library, or we could leave it as-is where the changes to the library come after the cmake changes. You tell me whichever you think is best.

[rbsheth]

@ned14 So this should be considered a patch release (like -p1) right? Why does your branch have v4.1.0? I don't see any versions past 2.0 upstream.

[ned14]

So this should be considered a patch release (like -p1) right?

Yes, it's why I chose -p1 for its branch name. As in it is v4.1.0 patch 1.

Why does your branch have v4.1.0? I don't see any versions past 2.0 upstream.

Yeah the original repo is weird. He seems to have stopped tagging releases past v2.0 for no obvious reason. There are open issues in his issue tracker complaining about the lack of up to date tags.

The version Ruslan uploaded here is the latest, v4.1 according to library source code. Ruslan set the correct version in cmake. I have no idea why he chose v0.0.0 for the hunter package branch name and default.cmake version.

Ruslan also chose -p1 for v0.0.0, again for unknown reasons. If you therefore think that 4.1.0-p2 is more appropriate, that works for me too.

[ned14]

@rbsheth Any further feedback? Can the PR to libs3 be merged now?

[rbsheth]

@ned14 Oops, this one slipped under the radar. Seems fine - I made a new branch and release here: https://github.com/cpp-pm/libs3/releases/tag/v4.1.0-287e4be-p0

[ned14]

I may need some help: why is the openssl dependency build failing on MSVC in https://ci.appveyor.com/project/ned14/hunter-testing? I've compared pkg.template branch to pkg.s3, they are identical except for where they really do need to be different. Obviously enough, it builds just fine on MSVC over here. Though I am on VS2019 instead of VS2017.

[caseymcc]

Openssl is only valid on VS2017 and VS2015 the others (ninja, nmake, mingw and msys) have known errors. https://github.com/cpp-pm/hunter-testing/blob/pkg.openssl/appveyor.yml

[ned14]

Openssl is only valid on VS2017 and VS2015 the others (ninja, nmake, mingw and msys) have known errors. https://github.com/cpp-pm/hunter-testing/blob/pkg.openssl/appveyor.yml

Did you actually mean to say that ninja, nmake, mingw, and msys don't work for OpenSSL on MSVC?

(I'm surprised that ninja doesn't work, we have here at work ninja + MSVC + OpenSSL from hunter working just fine)

[caseymcc]

Yes

Whoever setup openssl to work in hunter apparently had some issues that they chose not to resolve for ninja, nmake, mingw and msys. It may just be something with the appveyor's setup that was not fixed (or something deeper), I don't know. But just looking at the appveyor.yml shows that there were some issues. The links in the file will take you to the original failures if you think you might be able to correct the issues.

[ned14]

Cool, thanks for the info. Very useful. I have disabled those CI targets for Appveyor.

My next question is why the osx-10-13-cxx14 build is failing on Travis, when the preceding version of libs3 had that CI enabled. The cause appears to be CURL, which has no special casing for that CI target in its hunter-testing. So I am surprised.

https://travis-ci.org/github/ned14/hunter-testing/builds/715895378

[ned14]

Ok that's the PRs opened

[ned14]

All merged. Thanks everybody for your feedback!