From eef19aa7aa95174f6619777ec74f62d407f60f8a Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Fri, 2 Sep 2022 13:21:38 -0700 Subject: [PATCH 01/11] Add `$schema` to `cgmanifest.json` (#134) Co-authored-by: Jamie Magee --- licenses/cgmanifest.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/licenses/cgmanifest.json b/licenses/cgmanifest.json index a137381b..c4f46455 100644 --- a/licenses/cgmanifest.json +++ b/licenses/cgmanifest.json @@ -1,4 +1,5 @@ { + "$schema": "https://json.schemastore.org/component-detection-manifest.json", "Registrations": [ { "Component": { @@ -226,4 +227,4 @@ "DevelopmentDependency": false } ] -} \ No newline at end of file +} From 3591e381f96d80f054b13808a19af2353d02b954 Mon Sep 17 00:00:00 2001 From: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com> Date: Sun, 11 Sep 2022 02:27:07 +0530 Subject: [PATCH 02/11] Agent: Handle failures from callbacks during download (#136) * #62 lead to the finding that write failures from OnData callback were being ignored. This was leading to continued attempts to download even though data was not being written to disk (no free space). * HttpAgent now stops processing the response on callback failure and reports the callback error via OnComplete. * Fix pipeline build: Build agent now has IPv6 address which was unexpected in the rest interface test. --- client-lite/src/download/download.cpp | 52 ++++++--- client-lite/src/download/download.h | 3 +- client-lite/src/util/http_agent.cpp | 104 +++++++++--------- client-lite/src/util/http_agent.h | 1 + client-lite/src/util/http_agent_interface.h | 2 +- client-lite/test/CMakeLists.txt | 1 + client-lite/test/rest_http_listener_tests.cpp | 35 +----- common/CMakeLists.txt | 11 +- common/lib-dotestutil/CMakeLists.txt | 11 +- common/lib-dotestutil/do_test_helpers.cpp | 24 +++- common/lib-dotestutil/do_test_helpers.h | 8 +- sdk-cpp/tests/CMakeLists.txt | 6 +- sdk-cpp/tests/download_tests_common.cpp | 47 +++++++- sdk-cpp/tests/rest/rest_interface_tests.cpp | 3 +- sdk-cpp/tests/test_helpers.h | 34 +++++- 15 files changed, 220 insertions(+), 122 deletions(-) diff --git a/client-lite/src/download/download.cpp b/client-lite/src/download/download.cpp index 962bb0f4..05b16668 100644 --- a/client-lite/src/download/download.cpp +++ b/client-lite/src/download/download.cpp @@ -597,6 +597,30 @@ bool Download::_ShouldFailFastPerConnectionType() const } } +bool Download::_IsFatalError(HRESULT hrRequest, HRESULT hrCallback, UINT httpStatusCode) const +{ + if (FAILED(hrCallback)) + { + return true; + } + + if (HttpAgent::IsClientError(httpStatusCode) && _ShouldFailFastPerConnectionType()) + { + return true; + } + + switch (hrRequest) + { + case E_OUTOFMEMORY: + case INET_E_INVALID_URL: + case DO_E_INSUFFICIENT_RANGE_SUPPORT: + case HRESULT_FROM_XPLAT_SYSERR(CURLE_WRITE_ERROR): + return true; + } + + return false; +} + // IHttpAgentEvents HRESULT Download::OnHeadersAvailable() try @@ -641,11 +665,11 @@ HRESULT Download::OnData(_In_reads_bytes_(cbData) BYTE* pData, UINT cbData) try return S_OK; } CATCH_RETURN() -HRESULT Download::OnComplete(HRESULT hResult) +HRESULT Download::OnComplete(HRESULT hrRequest, HRESULT hrCallback) { try { - if (SUCCEEDED(hResult)) + if (SUCCEEDED(hrRequest)) { _taskThread.Sched([this]() { @@ -663,7 +687,7 @@ HRESULT Download::OnComplete(HRESULT hResult) LOG_IF_FAILED(_httpAgent->QueryStatusCode(&httpStatusCode)); LOG_IF_FAILED(_httpAgent->QueryHeaders(nullptr, responseHeaders)); - _taskThread.Sched([this, hResult, httpStatusCode, responseHeaders = std::move(responseHeaders)]() + _taskThread.Sched([this, hrRequest, hrCallback, httpStatusCode, responseHeaders = std::move(responseHeaders)]() { _fHttpRequestActive = false; _httpStatusCode = httpStatusCode; @@ -677,35 +701,35 @@ HRESULT Download::OnComplete(HRESULT hResult) if (_UsingMcc()) { - _mccManager.ReportHostError(hResult, _httpStatusCode, _mccHost, _url); + _mccManager.ReportHostError(hrRequest, _httpStatusCode, _mccHost, _url); } - const bool isFatalError = HttpAgent::IsClientError(_httpStatusCode); + const auto hrErrorToReport = FAILED(hrCallback) ? hrCallback : hrRequest; - // Fail fast on certain http errors - if (isFatalError && _ShouldFailFastPerConnectionType()) + // Fail fast on certain http/local errors + if (_IsFatalError(hrRequest, hrCallback, httpStatusCode)) { - DoLogInfoHr(hResult, "%s, fatal failure, http_status: %d, headers:\n%s", - GuidToString(_id).data(), _httpStatusCode, _responseHeaders.data()); + DoLogWarningHr(hrRequest, "%s, fatal failure, http_status: %d, hrCallback: 0x%x, headers:\n%s", + GuidToString(_id).data(), _httpStatusCode, hrCallback, _responseHeaders.data()); _Pause(); - _status._Paused(hResult); + _status._Paused(hrErrorToReport); return; } // Make note of the failure and stay in Transferring state for retry - _status.Error = hResult; + _status.Error = hrErrorToReport; _progressTracker.OnDownloadFailure(); std::chrono::seconds retryDelay = _progressTracker.NextRetryDelay(); // If we must fallback from MCC due to this error, retry without a delay - if (_ShouldPauseMccUsage(isFatalError)) + if (_ShouldPauseMccUsage(HttpAgent::IsClientError(_httpStatusCode))) { retryDelay = std::chrono::seconds(0); _progressTracker.ResetRetryDelay(); } - DoLogInfoHr(hResult, "%s, failure, will retry in %lld seconds, http_status: %d, headers:\n%s", - GuidToString(_id).data(), retryDelay.count(), _httpStatusCode, _responseHeaders.data()); + DoLogInfoHr(hrRequest, "%s, failure, will retry in %lld seconds, http_status: %d, hrCallback: 0x%x, headers:\n%s", + GuidToString(_id).data(), retryDelay.count(), _httpStatusCode, hrCallback, _responseHeaders.data()); _taskThread.Sched([this]() { // Nothing to do if we moved out of Transferring state in the meantime or diff --git a/client-lite/src/download/download.h b/client-lite/src/download/download.h index dfb5118f..83d1ca9d 100644 --- a/client-lite/src/download/download.h +++ b/client-lite/src/download/download.h @@ -149,9 +149,10 @@ class Download : public IHttpAgentEvents bool _UsingMcc() const { return _connectionType == ConnectionType::MCC; } bool _ShouldPauseMccUsage(bool isFatalError) const; bool _ShouldFailFastPerConnectionType() const; + bool _IsFatalError(HRESULT hrRequest, HRESULT hrCallback, UINT httpStatusCode) const; // IHttpAgentEvents HRESULT OnHeadersAvailable() override; HRESULT OnData(_In_reads_bytes_(cbData) BYTE* pData, UINT cbData) override; - HRESULT OnComplete(HRESULT hResult) override; + HRESULT OnComplete(HRESULT hrRequest, HRESULT hrCallback) override; }; diff --git a/client-lite/src/util/http_agent.cpp b/client-lite/src/util/http_agent.cpp index 0c7cc6d0..2944b5f9 100644 --- a/client-lite/src/util/http_agent.cpp +++ b/client-lite/src/util/http_agent.cpp @@ -88,6 +88,7 @@ HRESULT HttpAgent::SendRequest(PCSTR szUrl, PCSTR szProxyUrl, PCSTR szRange, UIN _requestContext.responseHeaders.clear(); _requestContext.responseStatusCode = 0; _requestContext.hrTranslatedStatusCode = S_OK; + _requestContext.hrCallback = S_OK; _requestContext.responseOnHeadersAvailableInvoked = false; _requestContext.responseOnCompleteInvoked = false; _curlOps.Add(_requestContext.curlHandle, s_CompleteCallback, this); @@ -357,12 +358,15 @@ size_t HttpAgent::_HeaderCallback(char* pBuffer, size_t size, size_t nItems) result.first->second.assign(pColon + 1, pBufferPastEnd); } } + return cbBuffer; } - catch (const std::exception& e) + catch (const std::bad_alloc&) { - DoLogWarning("Caught exception: %s", e.what()); + _requestContext.hrCallback = E_OUTOFMEMORY; } - return cbBuffer; + // Return value != cbBuffer causes curl to fail the request with CURLE_WRITE_ERROR. + // _CompleteCallback() will pass an appropriate error code to IHttpAgentEvents::OnComplete. + return 0; } size_t HttpAgent::_WriteCallback(char* pBuffer, size_t size, size_t nMemb) @@ -371,11 +375,40 @@ size_t HttpAgent::_WriteCallback(char* pBuffer, size_t size, size_t nMemb) const auto cbBuffer = nMemb * size; // Forward body only for success response - if (SUCCEEDED(_requestContext.hrTranslatedStatusCode)) + if (SUCCEEDED(_requestContext.hrTranslatedStatusCode) && SUCCEEDED(_requestContext.hrCallback)) + { + _requestContext.hrCallback = _callback.OnData(reinterpret_cast(pBuffer), cbBuffer); + } + + if (SUCCEEDED(_requestContext.hrTranslatedStatusCode) && SUCCEEDED(_requestContext.hrCallback)) + { + return cbBuffer; + } + else + { + // Return value != cbBuffer causes curl to fail the request with CURLE_WRITE_ERROR. + // _CompleteCallback() will pass an appropriate error code to IHttpAgentEvents::OnComplete. + return 0; + } +} + +static HRESULT g_CurlErrorCodeToHResult(int curlResult) +{ + switch (curlResult) { - (void)_callback.OnData(reinterpret_cast(pBuffer), cbBuffer); + case CURLE_URL_MALFORMAT: + return INET_E_INVALID_URL; + case CURLE_COULDNT_RESOLVE_HOST: + return HRESULT_FROM_WIN32(ERROR_WINHTTP_NAME_NOT_RESOLVED); + case CURLE_COULDNT_CONNECT: + return HRESULT_FROM_WIN32(ERROR_WINHTTP_CANNOT_CONNECT); + case CURLE_OPERATION_TIMEDOUT: + return WININET_E_TIMEOUT; + case CURLE_RANGE_ERROR: + return DO_E_INSUFFICIENT_RANGE_SUPPORT; + default: + return HRESULT_FROM_XPLAT_SYSERR(curlResult); } - return cbBuffer; } void HttpAgent::_CompleteCallback(int curlResult) @@ -388,42 +421,17 @@ void HttpAgent::_CompleteCallback(int curlResult) if (curlResult == CURLE_OK) { + // OnHeadersAvailable might not have been called earlier if response did not have a body _TrySetStatusCodeAndInvokeOnHeadersAvailable(); - - _requestContext.hrTranslatedStatusCode = _ResultFromStatusCode(_requestContext.responseStatusCode); - (void)_callback.OnComplete(_requestContext.hrTranslatedStatusCode); } - else + else if (SUCCEEDED(_requestContext.hrTranslatedStatusCode)) { - switch (curlResult) - { - case CURLE_URL_MALFORMAT: - _requestContext.hrTranslatedStatusCode = INET_E_INVALID_URL; - break; - - case CURLE_COULDNT_RESOLVE_HOST: - _requestContext.hrTranslatedStatusCode = HRESULT_FROM_WIN32(ERROR_WINHTTP_NAME_NOT_RESOLVED); - break; - - case CURLE_COULDNT_CONNECT: - _requestContext.hrTranslatedStatusCode = HRESULT_FROM_WIN32(ERROR_WINHTTP_CANNOT_CONNECT); - break; - - case CURLE_OPERATION_TIMEDOUT: - _requestContext.hrTranslatedStatusCode = WININET_E_TIMEOUT; - break; - - case CURLE_RANGE_ERROR: - _requestContext.hrTranslatedStatusCode = DO_E_INSUFFICIENT_RANGE_SUPPORT; - break; - - default: - _requestContext.hrTranslatedStatusCode = HRESULT_FROM_XPLAT_SYSERR(curlResult); - break; - } - - (void)_callback.OnComplete(_requestContext.hrTranslatedStatusCode); + // There are no prior errors to retain, report the current curl error code + _requestContext.hrTranslatedStatusCode = g_CurlErrorCodeToHResult(curlResult); } + // else we already have the error code to report + + (void)_callback.OnComplete(_requestContext.hrTranslatedStatusCode, _requestContext.hrCallback); } void HttpAgent::_TrySetStatusCodeAndInvokeOnHeadersAvailable() @@ -434,22 +442,20 @@ void HttpAgent::_TrySetStatusCodeAndInvokeOnHeadersAvailable() } _requestContext.responseOnHeadersAvailableInvoked = true; - long responseCode; - auto res = curl_easy_getinfo(_requestContext.curlHandle, CURLINFO_RESPONSE_CODE, &responseCode); - if (res == CURLE_OK) + long responseCode = 0; + auto curlResult = curl_easy_getinfo(_requestContext.curlHandle, CURLINFO_RESPONSE_CODE, &responseCode); + if (curlResult == CURLE_OK) { _requestContext.responseStatusCode = static_cast(responseCode); + _requestContext.hrTranslatedStatusCode = _ResultFromStatusCode(_requestContext.responseStatusCode); } - - _requestContext.hrTranslatedStatusCode = _ResultFromStatusCode(_requestContext.responseStatusCode); - if (SUCCEEDED(_requestContext.hrTranslatedStatusCode)) + else { - (void)_callback.OnHeadersAvailable(); + _requestContext.hrTranslatedStatusCode = HRESULT_FROM_XPLAT_SYSERR(curlResult); } - else + + if (SUCCEEDED(_requestContext.hrTranslatedStatusCode)) { - // Not interested in receiving body for failure response. Notify completion immediately. - _requestContext.responseOnCompleteInvoked = true; - (void)_callback.OnComplete(_requestContext.hrTranslatedStatusCode); + _requestContext.hrCallback = _callback.OnHeadersAvailable(); } } diff --git a/client-lite/src/util/http_agent.h b/client-lite/src/util/http_agent.h index 4d491e38..a4e77b6c 100644 --- a/client-lite/src/util/http_agent.h +++ b/client-lite/src/util/http_agent.h @@ -52,6 +52,7 @@ class HttpAgent : public IHttpAgent unsigned int responseStatusCode; HRESULT hrTranslatedStatusCode; + HRESULT hrCallback; std::unordered_map responseHeaders; bool responseOnHeadersAvailableInvoked; bool responseOnCompleteInvoked; diff --git a/client-lite/src/util/http_agent_interface.h b/client-lite/src/util/http_agent_interface.h index 6879ae91..47a0be44 100644 --- a/client-lite/src/util/http_agent_interface.h +++ b/client-lite/src/util/http_agent_interface.h @@ -27,5 +27,5 @@ class IHttpAgentEvents virtual ~IHttpAgentEvents() = default; virtual HRESULT OnHeadersAvailable() = 0; virtual HRESULT OnData(_In_reads_bytes_(cbData) BYTE* pData, UINT cbData) = 0; - virtual HRESULT OnComplete(HRESULT hResult) = 0; + virtual HRESULT OnComplete(HRESULT hrRequest, HRESULT hrCallback) = 0; }; diff --git a/client-lite/test/CMakeLists.txt b/client-lite/test/CMakeLists.txt index c54084a5..ea9db484 100644 --- a/client-lite/test/CMakeLists.txt +++ b/client-lite/test/CMakeLists.txt @@ -11,6 +11,7 @@ find_package(GTest REQUIRED) file (GLOB files_docs_tests *.cpp) add_executable (deliveryoptimization-agent-tests ${files_docs_tests}) +add_platform_interface_definitions(deliveryoptimization-agent-tests) target_link_libraries(deliveryoptimization-agent-tests docs_common dotestutil diff --git a/client-lite/test/rest_http_listener_tests.cpp b/client-lite/test/rest_http_listener_tests.cpp index e71d0ac6..592e49ac 100644 --- a/client-lite/test/rest_http_listener_tests.cpp +++ b/client-lite/test/rest_http_listener_tests.cpp @@ -4,44 +4,17 @@ #include "test_common.h" #include "rest_http_listener.h" -#include -#include +#include "do_test_helpers.h" using btcp_t = boost::asio::ip::tcp; using bsock_t = boost::asio::ip::tcp::socket; -class BoostAsioWorker -{ -public: - BoostAsioWorker() : - _work(_io) - { - _myThread = std::thread([this]() - { - _io.run(); - }); - } - - boost::asio::io_service& IoService() { return _io; } - - ~BoostAsioWorker() - { - _io.stop(); - _myThread.join(); - } - -private: - boost::asio::io_service _io; - boost::asio::io_service::work _work; - std::thread _myThread; -}; - TEST(RestListenerTests, ManyPortsInUse) { // Listener searches for ports from 50000 to 60999. // We try to block all ports until 54999 and check time taken for RestHttpListener::Start(). - BoostAsioWorker asioWorker; + dotest::util::BoostAsioWorker asioWorker; // Create and bind as many sockets as possible (usual limit is 1024 total open file descriptors) std::vector sockets; @@ -50,7 +23,7 @@ TEST(RestListenerTests, ManyPortsInUse) { try { - bsock_t sock(asioWorker.IoService()); + bsock_t sock(asioWorker.Service()); sock.open(btcp_t::v4()); sock.bind(btcp_t::endpoint(btcp_t::v4(), port)); sockets.push_back(std::move(sock)); @@ -87,7 +60,7 @@ TEST(RestListenerTests, ManyPortsInUse) RestHttpListener listener; const auto before = std::chrono::steady_clock::now(); - listener.Start(asioWorker.IoService(), http_listener_callback_t{}); + listener.Start(asioWorker.Service(), http_listener_callback_t{}); const auto after = std::chrono::steady_clock::now(); std::cout << "Listener started at: " << listener.Endpoint() << "\n"; const auto elapsedMsecs = std::chrono::duration_cast(after - before).count(); diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt index aac831e6..285cc267 100644 --- a/common/CMakeLists.txt +++ b/common/CMakeLists.txt @@ -3,11 +3,12 @@ if (DO_PLATFORM_LINUX OR DO_PLATFORM_MAC) add_subdirectory(lib-dohttp) - if (DO_BUILD_TESTS) - # Plugins subproject doesn't have test code - if (DO_INCLUDE_AGENT OR DO_INCLUDE_SDK) - add_subdirectory(lib-dotestutil) - endif() +endif() + +if (DO_BUILD_TESTS) + # Plugins subproject doesn't have test code + if (DO_INCLUDE_AGENT OR DO_INCLUDE_SDK) + add_subdirectory(lib-dotestutil) endif() endif() diff --git a/common/lib-dotestutil/CMakeLists.txt b/common/lib-dotestutil/CMakeLists.txt index 47f84d75..52017aa0 100644 --- a/common/lib-dotestutil/CMakeLists.txt +++ b/common/lib-dotestutil/CMakeLists.txt @@ -3,10 +3,13 @@ set(test_lib_name dotestutil) +find_package(Boost COMPONENTS system) # for asio + add_library(${test_lib_name} STATIC do_test_helpers.cpp) -target_include_directories(${test_lib_name} - INTERFACE - ${CMAKE_CURRENT_SOURCE_DIR} - PRIVATE +add_platform_interface_definitions(${test_lib_name}) +target_include_directories(${test_lib_name} + PUBLIC ${Boost_INCLUDE_DIRS} #boost/asio.hpp + INTERFACE + ${CMAKE_CURRENT_SOURCE_DIR} ) \ No newline at end of file diff --git a/common/lib-dotestutil/do_test_helpers.cpp b/common/lib-dotestutil/do_test_helpers.cpp index b2d30e54..3bd42fb5 100644 --- a/common/lib-dotestutil/do_test_helpers.cpp +++ b/common/lib-dotestutil/do_test_helpers.cpp @@ -13,6 +13,9 @@ namespace util void ExecuteSystemCommand(const char* command) { +#ifdef DO_PLATFORM_WINDOWS + throw std::exception("System command not implemented"); +#else int ret = system(command); if (!WIFEXITED(ret)) { @@ -24,8 +27,10 @@ void ExecuteSystemCommand(const char* command) { throw DOTestException("Command [%s] completed with error: %d", command, cmdRet); } +#endif } +#ifdef DO_PLATFORM_LINUX using btcp_t = boost::asio::ip::tcp; BoostAsioWorker::~BoostAsioWorker() @@ -35,16 +40,17 @@ BoostAsioWorker::~BoostAsioWorker() } // Returns nullptr if no address was found for the DNS query -std::unique_ptr BoostAsioWorker::ResolveDnsQuery(const btcp_t::resolver::query& resolverQuery) +std::unique_ptr BoostAsioWorker::ResolveDnsQuery(const btcp_t::resolver::query& resolverQuery, + const boost::asio::ip::tcp::resolver::protocol_type* prot) { std::promise> epPromise; auto fut = epPromise.get_future(); - auto fnResolveHandler = [&epPromise](const boost::system::error_code& ec, btcp_t::resolver::iterator endpoints) -> void + auto fnResolveHandler = [&epPromise, prot](const boost::system::error_code& ec, btcp_t::resolver::iterator endpoints) -> void { std::unique_ptr spFoundEp; if (ec) { - std::cout << FormatString("Error resolving address: %d, %s\n", ec.value(), ec.message().data()); + std::cout << FormatString("Error resolving address: %d, %s\n", ec.value(), ec.message().c_str()); } else if (endpoints == btcp_t::resolver::iterator()) { @@ -56,10 +62,17 @@ std::unique_ptr BoostAsioWorker::ResolveDnsQuery(const btcp_t: while (endpoints != btcp_t::resolver::iterator()) { const auto& ep = *endpoints++; - std::cout << FormatString("Host: %s, IP: %s\n", ep.host_name().data(), ep.endpoint().address().to_string().data()); - spFoundEp = std::make_unique(ep); + std::cout << FormatString("Host: %s, IP: %s\n", ep.host_name().data(), ep.endpoint().address().to_string().c_str()); + if ((prot == nullptr) || (ep.endpoint().protocol() == *prot)) + { + spFoundEp = std::make_unique(ep); + } } } + if (spFoundEp) + { + std::cout << FormatString("Returning IP: %s\n", spFoundEp->address().to_string().c_str()); + } epPromise.set_value(std::move(spFoundEp)); }; @@ -74,6 +87,7 @@ std::unique_ptr BoostAsioWorker::ResolveDnsQuery(const btcp_t: } return fut.get(); } +#endif // DO_PLATFORM_LINUX } // namespace util } // namespace dotest diff --git a/common/lib-dotestutil/do_test_helpers.h b/common/lib-dotestutil/do_test_helpers.h index df57dd39..0e55ddd9 100644 --- a/common/lib-dotestutil/do_test_helpers.h +++ b/common/lib-dotestutil/do_test_helpers.h @@ -5,7 +5,10 @@ #include #include +#ifdef DO_PLATFORM_LINUX +// TODO(shishirb) Need to install boost-asio on windows/mac build agents? #include +#endif namespace dotest { @@ -26,12 +29,14 @@ std::string FormatString(const char* fmt, Args&&... args) // Helper classes +#ifdef DO_PLATFORM_LINUX class BoostAsioWorker { public: ~BoostAsioWorker(); - std::unique_ptr ResolveDnsQuery(const boost::asio::ip::tcp::resolver::query& resolverQuery); + std::unique_ptr ResolveDnsQuery(const boost::asio::ip::tcp::resolver::query& resolverQuery, + const boost::asio::ip::tcp::resolver::protocol_type* prot = nullptr); boost::asio::io_service& Service() { return _io; } @@ -40,6 +45,7 @@ class BoostAsioWorker boost::asio::io_service::work _work { _io }; std::thread _myThread { [this](){ _io.run(); } }; }; +#endif // DO_PLATFORM_LINUX class DOTestException : public std::exception { diff --git a/sdk-cpp/tests/CMakeLists.txt b/sdk-cpp/tests/CMakeLists.txt index e854001d..f755535f 100644 --- a/sdk-cpp/tests/CMakeLists.txt +++ b/sdk-cpp/tests/CMakeLists.txt @@ -10,6 +10,7 @@ set(sdk_tests_linked_libs_common Microsoft::deliveryoptimization ${Boost_LIBRARIES} GTest::GTest + dotestutil ) set(dosdkcpp_private_includes_common @@ -31,7 +32,6 @@ if(DO_PLATFORM_LINUX) set(sdk_tests_linked_libs stdc++fs - dotestutil ) set(test_source_private @@ -45,10 +45,6 @@ elseif (DO_PLATFORM_MAC) "../src/internal/rest/util" ) - set(sdk_tests_linked_libs - dotestutil - ) - # Many of the rest interface tests test simple client specific functionality, should make a seperate folder for simple client set(test_source_private "rest/test_helpers.cpp" diff --git a/sdk-cpp/tests/download_tests_common.cpp b/sdk-cpp/tests/download_tests_common.cpp index f053f0b8..f5873b5d 100644 --- a/sdk-cpp/tests/download_tests_common.cpp +++ b/sdk-cpp/tests/download_tests_common.cpp @@ -3,6 +3,10 @@ #include "tests_common.h" +#ifdef DO_CLIENT_DOSVC +#include // HRESULT_FROM_WIN32, ERROR_FILE_NOT_FOUND +#endif + #include #include #include @@ -16,9 +20,7 @@ #include "do_download.h" #include "do_download_status.h" #include "do_errors.h" -#if defined(DO_INTERFACE_REST) #include "do_test_helpers.h" -#endif #include "test_data.h" #include "test_helpers.h" @@ -553,7 +555,45 @@ TEST_F(DownloadTests, MultipleConcurrentDownloadTest_WithCancels) ASSERT_EQ(boost::filesystem::file_size(boost::filesystem::path(g_tmpFileName3)), g_smallFileSizeBytes); } +TEST_F(DownloadTests, FileDeletionAfterPause) +{ + auto largeDownload = msdot::download::make(g_largeFileUrl, g_tmpFileName2); + auto cleanup = dotest::util::scope_exit([&largeDownload]() + { + std::cout << "Aborting download\n"; + largeDownload->abort(); + }); + + largeDownload->start(); + std::this_thread::sleep_for(2s); + largeDownload->pause(); + auto status = largeDownload->get_status(); + ASSERT_EQ(status.state(), msdo::download_state::paused) << "Download is paused"; + + boost::filesystem::remove(g_tmpFileName2); + ASSERT_FALSE(boost::filesystem::exists(g_tmpFileName2)) << "Output file deleted"; + +#if defined(DO_CLIENT_AGENT) + try + { + largeDownload->resume(); + ASSERT_TRUE(false) << "Expected resume() to throw"; + } + catch (const msdod::exception& ex) + { + ASSERT_EQ(ex.error_code().value(), DO_ERROR_FROM_SYSTEM_ERROR(ENOENT)) << "Resume failed due to missing output file"; + } +#else + TestHelpers::DeleteDoSvcTemporaryFiles(g_tmpFileName2); + largeDownload->resume(); + TestHelpers::WaitForState(*largeDownload, msdo::download_state::paused); + status = largeDownload->get_status(); + ASSERT_EQ(status.error_code().value(), HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)); +#endif +} + #if defined(DO_INTERFACE_REST) + TEST_F(DownloadTests, SimpleBlockingDownloadTest_ClientNotRunning) { TestHelpers::StopService("deliveryoptimization-agent.service"); @@ -619,5 +659,4 @@ TEST_F(DownloadTests, MultipleRestPortFileExists_Download) ASSERT_EQ(boost::filesystem::file_size(boost::filesystem::path(g_tmpFileName)), g_smallFileSizeBytes); } - -#endif // Linux +#endif // DO_INTERFACE_REST diff --git a/sdk-cpp/tests/rest/rest_interface_tests.cpp b/sdk-cpp/tests/rest/rest_interface_tests.cpp index 32339b57..3570de6b 100644 --- a/sdk-cpp/tests/rest/rest_interface_tests.cpp +++ b/sdk-cpp/tests/rest/rest_interface_tests.cpp @@ -83,7 +83,8 @@ TEST_F(RestInterfaceTests, RestInterfaceUseLocalHostForLocalSocket) const auto localHostname = boost::asio::ip::host_name(); btcp_t::resolver::query query(localHostname, ""); - auto spLocalEp = asioService.ResolveDnsQuery(query); + auto prot = btcp_t::v4(); + auto spLocalEp = asioService.ResolveDnsQuery(query, &prot); ASSERT_TRUE(spLocalEp) << "Found at least one address for the local hostname query"; // Hostname can resolve to either a loopback address or private address depending on machine/network config diff --git a/sdk-cpp/tests/test_helpers.h b/sdk-cpp/tests/test_helpers.h index d786068a..b650178b 100644 --- a/sdk-cpp/tests/test_helpers.h +++ b/sdk-cpp/tests/test_helpers.h @@ -12,6 +12,7 @@ #include "do_download.h" #include "do_download_status.h" #include "do_error_helpers.h" +#include "do_test_helpers.h" namespace msdo = microsoft::deliveryoptimization; namespace msdod = microsoft::deliveryoptimization::details; @@ -145,6 +146,32 @@ class TestHelpers } } + // DoSvc creates temporary files with a unique name in the same directory as the output file + static void DeleteDoSvcTemporaryFiles(const boost::filesystem::path& outputFilePath) + { + const boost::filesystem::path parentDir = outputFilePath.parent_path(); + for (boost::filesystem::directory_iterator itr(parentDir); itr != boost::filesystem::directory_iterator(); ++itr) + { + const boost::filesystem::directory_entry& dirEntry = *itr; + // Remove all files with names that match DO*.tmp + if (boost::filesystem::is_regular_file(dirEntry) + && (dirEntry.path().filename().string().find("DO") == 0) + && (dirEntry.path().extension() == ".tmp")) + { + boost::system::error_code ec; + boost::filesystem::remove(dirEntry, ec); + if (ec) + { + std::cout << "Temp file deletion error: " << ec.message() << ", " << dirEntry.path() << '\n'; + } + else + { + std::cout << "Deleted DoSvc temp file: " << dirEntry.path() << '\n'; + } + } + } + } + // On Windows, operations are async - there may be some delay setting a state internally static void WaitForState(microsoft::deliveryoptimization::test::download& download, msdo::download_state expectedState, std::chrono::seconds waitTimeSecs = 10s) @@ -158,7 +185,12 @@ class TestHelpers std::cout << "Transferred " << status.bytes_transferred() << " / " << status.bytes_total() << "\n"; } - ASSERT_EQ(status.state(), expectedState) << "Download must have reached expected state before timeout"; + if (status.state() != expectedState) + { + // Throw exception instead of ASSERT* to let tests catch if needed + const auto msg = dotest::util::FormatString("State: expected = %d, actual = %d", expectedState, status.state()); + throw std::runtime_error(msg); + } } #ifdef DO_INTERFACE_REST From 21d94248eb13a3644c948a264b7d649addcc7851 Mon Sep 17 00:00:00 2001 From: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com> Date: Thu, 15 Sep 2022 12:00:57 +0530 Subject: [PATCH 03/11] SDK: Fix issues seen in MSEdge build (#137) * Fix returning address of stack variable. * Fix extra semi colons after method definition. * Enabling building with clang via Visual Studio to replicate some of MSEdge build. * Fix 'no exceptions' build and some warnings. * Fix test code for MacOS build. * Fix windows build pipeline: vcpkg, after build agent updates. --- .gitignore | 1 + CMakeLists.txt | 20 +++++++++- CMakeSettings.json | 38 +++++++++++++++++++ .../windows/templates/dosdkcpp-steps.yml | 28 ++++++++++++-- build/scripts/install-vcpkg-deps.ps1 | 14 +++---- sdk-cpp/CMakeLists.txt | 4 ++ sdk-cpp/src/do_download.cpp | 2 +- sdk-cpp/src/do_download_property.cpp | 10 ++--- .../src/internal/com/do_config_internal.cpp | 2 +- .../com/do_download_property_internal.cpp | 12 +++--- sdk-cpp/src/internal/com/download_impl.cpp | 2 +- sdk-cpp/src/internal/do_error_helpers.h | 12 +++++- .../rest/do_download_property_internal.cpp | 30 +++++++-------- sdk-cpp/tests/CMakeLists.txt | 2 + sdk-cpp/tests/download_properties_tests.cpp | 4 +- sdk-cpp/tests/download_tests_common.cpp | 12 ++---- sdk-cpp/tests/test_helpers.h | 5 +-- 17 files changed, 141 insertions(+), 57 deletions(-) create mode 100644 CMakeSettings.json diff --git a/.gitignore b/.gitignore index 234e46b8..f75b3862 100644 --- a/.gitignore +++ b/.gitignore @@ -55,3 +55,4 @@ CTestTestfile.cmake _deps *_CPack_Packages/ *.deb +out/ diff --git a/CMakeLists.txt b/CMakeLists.txt index d74acd68..698ee041 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -71,13 +71,31 @@ set(CMAKE_POSITION_INDEPENDENT_CODE ON) if (DO_PLATFORM_LINUX) # PIE (Position Independent Executable) ensures exe/.so can run when ASLR is enabled in the target OS set(COMPILER_HARDENING_FLAGS - "-fPIE -pie -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wformat -Werror=format-security") + "-fPIE -pie -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wformat -Werror=format-security -Wreturn-local-addr") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall ${COMPILER_HARDENING_FLAGS} -fmerge-all-constants") # relro+now thwarts some attack vectors by reordering some ELF data structures and also by making the GOT read-only set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -pie -z relro -z now") endif (DO_PLATFORM_LINUX) +if (DO_PLATFORM_WINDOWS AND DO_INCLUDE_SDK) + # Replicate some of MSEdge build config to catch issues before integration. + # Use Visual Studio 2019 and the x64-Clang-Debug-SDK cmake config to enable these configs. + if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") + set(DISABLE_WARNINGS "-Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-reserved-id-macro -Wno-global-constructors -Wno-exit-time-destructors -Wno-extra-semi-stmt") + # Note: Many warnings to fix before we can turn on -Werror + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wreturn-stack-address -Wno-string-conversion ${DISABLE_WARNINGS}") + endif () + if (NOT DO_BUILD_TESTS) + # Disable C++ exceptions + string(REGEX REPLACE "/EH[a-z]+" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS}) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /D_HAS_EXCEPTIONS=0") + # Disable RTTI + string(REGEX REPLACE "/GR" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS}) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /GR-") + endif () +endif () + # Define DEBUG macro in debug builds string(TOLOWER ${CMAKE_BUILD_TYPE} DO_BUILD_TYPE) if (DO_BUILD_TYPE MATCHES debug) diff --git a/CMakeSettings.json b/CMakeSettings.json new file mode 100644 index 00000000..1f2f8972 --- /dev/null +++ b/CMakeSettings.json @@ -0,0 +1,38 @@ +{ + "configurations": [ + { + "name": "x64-Debug", + "generator": "Ninja", + "configurationType": "Debug", + "inheritEnvironments": [ "msvc_x64_x64" ], + "buildRoot": "${projectDir}\\out\\build\\${name}", + "installRoot": "${projectDir}\\out\\install\\${name}", + "cmakeCommandArgs": "", + "buildCommandArgs": "", + "ctestCommandArgs": "" + }, + { + "name": "x64-Clang-Debug-SDK", + "generator": "Ninja", + "configurationType": "Debug", + "buildRoot": "${projectDir}\\out\\build\\${name}", + "installRoot": "${projectDir}\\out\\install\\${name}", + "cmakeCommandArgs": "", + "buildCommandArgs": "", + "ctestCommandArgs": "", + "inheritEnvironments": [ "clang_cl_x64_x64" ], + "variables": [ + { + "name": "DO_INCLUDE_SDK", + "value": "True", + "type": "BOOL" + }, + { + "name": "DO_BUILD_TESTS", + "value": "False", + "type": "BOOL" + } + ] + } + ] +} \ No newline at end of file diff --git a/azure-pipelines/build/windows/templates/dosdkcpp-steps.yml b/azure-pipelines/build/windows/templates/dosdkcpp-steps.yml index 7ee1ae7a..3a17eab9 100644 --- a/azure-pipelines/build/windows/templates/dosdkcpp-steps.yml +++ b/azure-pipelines/build/windows/templates/dosdkcpp-steps.yml @@ -8,7 +8,7 @@ parameters: type: string - name: vcpkgDir type: string - default: $(Agent.TempDirectory)\deliveryoptimization_tools + default: $(Agent.TempDirectory)\deliveryoptimization_tools\vcpkg - name: dependencyScriptsLocation type: string default: $(Build.SourcesDirectory)/build/scripts @@ -17,12 +17,32 @@ parameters: default: false steps: +- task: PowerShell@2 + displayName: 'Find or install vcpkg' + inputs: + targetType: 'inline' + script: | + if (Test-Path "$env:VCPKG_ROOT\vcpkg.exe") + { + # Must be a version of pipeline agent that has vcpkg pre-installed + Write-Host "Found existing vcpkg in $env:VCPKG_ROOT" + } + else + { + $vcpkgInstallDir = ${{parameters.vcpkgDir}} + Write-Host "Installing vcpkg to $vcpkgInstallDir" + git clone https://github.com/microsoft/vcpkg $vcpkgInstallDir + # Build pipeline agents usually have both these defined + Write-Host "##vso[task.setvariable variable=VCPKG_ROOT;isoutput=true;isreadonly=true;]$vcpkgInstallDir" + Write-Host "##vso[task.setvariable variable=VCPKG_INSTALLATION_ROOT;isoutput=true;isreadonly=true;]$vcpkgInstallDir" + } + # Key input is hashed to compute the cache key so when the contents of install-vcpkg-deps.ps1 changes, a new key will be generated # This way, anytime we modify dependencies from install-vcpkg-deps.ps1 script, we don't retrieve from the old cache - task: Cache@2 inputs: key: '${{parameters.dependencyScriptsLocation}}/install-vcpkg-deps.ps1 | "${{parameters.targetOsArch}}"' - path: ${{parameters.vcpkgDir}} + path: $(VCPKG_ROOT) cacheHitVar: CACHE_RESTORED - task: PowerShell@2 @@ -30,14 +50,14 @@ steps: inputs: targetType: 'filePath' filePath: ${{parameters.dependencyScriptsLocation}}\install-vcpkg-deps.ps1 - arguments: ${{parameters.vcpkgDir}} + arguments: $(VCPKG_ROOT) displayName: 'Install vcpkg dependencies' - task: PythonScript@0 inputs: scriptSource: 'filePath' scriptPath: 'build/build.py' - arguments: '--project sdk --config ${{parameters.config}} --vcpkgdir ${{parameters.vcpkgDir}}\vcpkg --clean' + arguments: '--project sdk --config ${{parameters.config}} --vcpkgdir $(VCPKG_ROOT) --clean' displayName: 'Build sdk-cpp ${{parameters.targetOsArch}}-${{parameters.config}}' - task: CmdLine@2 diff --git a/build/scripts/install-vcpkg-deps.ps1 b/build/scripts/install-vcpkg-deps.ps1 index dc2a6b58..b8bc2032 100644 --- a/build/scripts/install-vcpkg-deps.ps1 +++ b/build/scripts/install-vcpkg-deps.ps1 @@ -1,14 +1,13 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -$vcpkgdir=$args[0] # path to install vcpkgdir, i.e. C:\users\user\appdata\local\temp +if (-not (Test-Path "$env:VCPKG_ROOT\vcpkg.exe")) +{ + Write-Host "vcpkg not found in $env:VCPKG_ROOT" + exit 1 +} -Write-Host "Installing vcpkg to $vcpkgdir" - -# You can also use the submoduled vcpkg directory within this project which comes with all necessary binaries pre-built -git clone https://github.com/microsoft/vcpkg $vcpkgdir\vcpkg - -cd $vcpkgdir\vcpkg +cd $env:VCPKG_ROOT git checkout 2021.05.12 .\bootstrap-vcpkg.bat .\vcpkg integrate install @@ -16,4 +15,3 @@ git checkout 2021.05.12 .\vcpkg install gtest:x64-windows .\vcpkg install boost-filesystem:x64-windows .\vcpkg install boost-program-options:x64-windows - diff --git a/sdk-cpp/CMakeLists.txt b/sdk-cpp/CMakeLists.txt index 56ebdbf3..a451309c 100644 --- a/sdk-cpp/CMakeLists.txt +++ b/sdk-cpp/CMakeLists.txt @@ -124,6 +124,10 @@ target_compile_definitions(${DO_SDK_LIB_NAME} if (DO_DEV_DEBUG) target_compile_definitions(${DO_SDK_LIB_NAME} PRIVATE DO_DEV_DEBUG) endif () +if (DO_PLATFORM_LINUX OR DO_PLATFORM_MAC) + # TODO(shishirb) Remove internal use of exceptions on these platforms + target_compile_definitions(${DO_SDK_LIB_NAME} PRIVATE DO_ENABLE_EXCEPTIONS) +endif () add_platform_interface_definitions(${DO_SDK_LIB_NAME}) target_link_libraries(${DO_SDK_LIB_NAME} diff --git a/sdk-cpp/src/do_download.cpp b/sdk-cpp/src/do_download.cpp index 740bc5a4..966d9ebe 100644 --- a/sdk-cpp/src/do_download.cpp +++ b/sdk-cpp/src/do_download.cpp @@ -142,7 +142,7 @@ std::error_code download::download_url_to_path(const std::string& uri, const std static std::error_code g_TryOverrideDownlevelOsSetPropertyError(download_property prop, std::error_code ec) { - // Temporary backward-compatibility for Chromium/Edge. + // Temporary backward-compatibility for MSEdge. // These properties were not supported in IDODownload interface until build 19041. if ((ec.value() == static_cast(errc::do_e_unknown_property_id)) && ((prop == download_property::correlation_vector) || (prop == download_property::integrity_check_info))) diff --git a/sdk-cpp/src/do_download_property.cpp b/sdk-cpp/src/do_download_property.cpp index ed49faa8..bd1043aa 100644 --- a/sdk-cpp/src/do_download_property.cpp +++ b/sdk-cpp/src/do_download_property.cpp @@ -81,22 +81,22 @@ std::error_code download_property_value::make(const status_callback_t& val, down std::error_code download_property_value::as(bool& val) const noexcept { return _val->As(val); -}; +} std::error_code download_property_value::as(uint32_t& val) const noexcept { return _val->As(val); -}; +} std::error_code download_property_value::as(uint64_t& val) const noexcept { return _val->As(val); -}; +} std::error_code download_property_value::as(std::string& val) const noexcept { return _val->As(val); -}; +} std::error_code download_property_value::as(std::vector& val) const noexcept { @@ -106,7 +106,7 @@ std::error_code download_property_value::as(std::vector& val) con std::error_code download_property_value::as(status_callback_t& val) const noexcept { return _val->As(val); -}; +} } // deliveryoptimization } // microsoft diff --git a/sdk-cpp/src/internal/com/do_config_internal.cpp b/sdk-cpp/src/internal/com/do_config_internal.cpp index 94569df8..af2b95c6 100644 --- a/sdk-cpp/src/internal/com/do_config_internal.cpp +++ b/sdk-cpp/src/internal/com/do_config_internal.cpp @@ -20,6 +20,6 @@ void internal_free_version_buf(char** ppBuffer) if (*ppBuffer) { free(*ppBuffer); - *ppBuffer = NULL; + *ppBuffer = nullptr; } } diff --git a/sdk-cpp/src/internal/com/do_download_property_internal.cpp b/sdk-cpp/src/internal/com/do_download_property_internal.cpp index 950653fd..233c5a89 100644 --- a/sdk-cpp/src/internal/com/do_download_property_internal.cpp +++ b/sdk-cpp/src/internal/com/do_download_property_internal.cpp @@ -19,7 +19,7 @@ namespace deliveryoptimization namespace details { -std::error_code UTF8toWstr(const char* str, std::wstring& wstr) +static std::error_code UTF8toWstr(const char* str, std::wstring& wstr) { size_t cch = strlen(str); if (cch == 0) @@ -28,12 +28,12 @@ std::error_code UTF8toWstr(const char* str, std::wstring& wstr) } std::vector dest(cch * 4); - const uint32_t result = MultiByteToWideChar(CP_UTF8, 0, str, static_cast(cch), dest.data(), static_cast(dest.size())); + const int result = MultiByteToWideChar(CP_UTF8, 0, str, static_cast(cch), dest.data(), static_cast(dest.size())); if (result == 0) { - return make_error_code(E_FAIL); + return make_error_code(HRESULT_FROM_WIN32(::GetLastError())); } - wstr = std::wstring(dest.data(), result); + wstr = std::wstring(dest.data(), static_cast(result)); return DO_OK; } @@ -103,13 +103,13 @@ CDownloadPropertyValueInternal::~CDownloadPropertyValueInternal() CDownloadPropertyValueInternal::CDownloadPropertyValueInternal(const CDownloadPropertyValueInternal& rhs) { - int32_t res = VariantCopy(&_var, &rhs._var); + HRESULT res = VariantCopy(&_var, &rhs._var); #if DEBUG assert(SUCCEEDED(res)); #endif if (FAILED(res)) { - throw std::bad_alloc(); + std::terminate(); } _callback = rhs._callback; }; diff --git a/sdk-cpp/src/internal/com/download_impl.cpp b/sdk-cpp/src/internal/com/download_impl.cpp index af336ae3..b06e8f35 100644 --- a/sdk-cpp/src/internal/com/download_impl.cpp +++ b/sdk-cpp/src/internal/com/download_impl.cpp @@ -205,7 +205,7 @@ class DOStatusCallback : return S_OK; } - IFACEMETHODIMP OnStatusChange(IDODownload* download, const DO_DOWNLOAD_STATUS* comStatus) noexcept + IFACEMETHODIMP OnStatusChange(IDODownload*, const DO_DOWNLOAD_STATUS* comStatus) noexcept override { msdo::download_status status = ConvertFromComStatus(*comStatus); _callback(*_download, status); diff --git a/sdk-cpp/src/internal/do_error_helpers.h b/sdk-cpp/src/internal/do_error_helpers.h index 87c02806..db5055fb 100644 --- a/sdk-cpp/src/internal/do_error_helpers.h +++ b/sdk-cpp/src/internal/do_error_helpers.h @@ -53,11 +53,14 @@ inline std::error_code make_error_code(errc e) return std::error_code(static_cast(e), do_category()); } +#ifdef DO_ENABLE_EXCEPTIONS + class exception : public std::exception { public: exception(std::error_code code) : - _code(std::move(code)) + _code(code), + _msg(code.message()) { } @@ -73,7 +76,7 @@ class exception : public std::exception const char* what() const noexcept override { - return _code.message().c_str(); + return _msg.c_str(); } const std::error_code& error_code() const @@ -83,6 +86,9 @@ class exception : public std::exception private: std::error_code _code; + + // std::error_code::message() has a by-value return. Store it here for implementation of what(). + std::string _msg; }; inline void throw_if_fail(std::error_code errorCode) @@ -113,6 +119,8 @@ inline void ThrowException(errc errorCode) throw exception(errorCode); } +#endif // DO_ENABLE_EXCEPTIONS + } // namespace details } //namespace deliveryoptimization } //namespace microsoft diff --git a/sdk-cpp/src/internal/rest/do_download_property_internal.cpp b/sdk-cpp/src/internal/rest/do_download_property_internal.cpp index 1e28ea92..b53d2716 100644 --- a/sdk-cpp/src/internal/rest/do_download_property_internal.cpp +++ b/sdk-cpp/src/internal/rest/do_download_property_internal.cpp @@ -23,27 +23,27 @@ CDownloadPropertyValueInternal::CDownloadPropertyValueInternal() = default; std::error_code CDownloadPropertyValueInternal::Init(const std::string& val) noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::Init(uint32_t val) noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::Init(uint64_t val) noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::Init(bool val) noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::Init(std::vector& val) noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::Init(const download_property_value::status_callback_t& val) noexcept { @@ -52,51 +52,51 @@ std::error_code CDownloadPropertyValueInternal::Init(const download_property_val CDownloadPropertyValueInternal::~CDownloadPropertyValueInternal() { -}; +} CDownloadPropertyValueInternal::CDownloadPropertyValueInternal(const CDownloadPropertyValueInternal& rhs) { _var = rhs._var; _callback = rhs._callback; -}; +} CDownloadPropertyValueInternal& CDownloadPropertyValueInternal::operator=(CDownloadPropertyValueInternal copy) { swap(*this, copy); return *this; -}; +} CDownloadPropertyValueInternal::CDownloadPropertyValueInternal(CDownloadPropertyValueInternal&& rhs) noexcept { _var = rhs._var; rhs._var = {}; _callback = std::move(rhs._callback); -}; +} const CDownloadPropertyValueInternal::native_type& CDownloadPropertyValueInternal::native_value() const noexcept { return _var; -}; +} std::error_code CDownloadPropertyValueInternal::As(bool& val) const noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::As(uint32_t& val) const noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::As(uint64_t& val) const noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::As(std::string& val) const noexcept { return make_error_code(errc::e_not_impl); -}; +} std::error_code CDownloadPropertyValueInternal::As(std::vector& val) const noexcept { @@ -107,7 +107,7 @@ std::error_code CDownloadPropertyValueInternal::As(download_property_value::stat { val = _callback; return DO_OK; -}; +} } // namespace details } // namespace deliveryoptimization diff --git a/sdk-cpp/tests/CMakeLists.txt b/sdk-cpp/tests/CMakeLists.txt index f755535f..e5d3fccc 100644 --- a/sdk-cpp/tests/CMakeLists.txt +++ b/sdk-cpp/tests/CMakeLists.txt @@ -65,6 +65,8 @@ file (GLOB test_source ) add_executable(deliveryoptimization-sdk-tests ${test_source}) +# Tests make use of C++ exceptions. MSEdge build on Windows disables C++ exceptions but also does not build our tests. +target_compile_definitions(deliveryoptimization-sdk-tests PRIVATE DO_ENABLE_EXCEPTIONS) add_platform_interface_definitions(deliveryoptimization-sdk-tests) target_include_directories(deliveryoptimization-sdk-tests diff --git a/sdk-cpp/tests/download_properties_tests.cpp b/sdk-cpp/tests/download_properties_tests.cpp index 37764422..3afe3df2 100644 --- a/sdk-cpp/tests/download_properties_tests.cpp +++ b/sdk-cpp/tests/download_properties_tests.cpp @@ -165,7 +165,7 @@ TEST_F(DownloadPropertyTests, CallbackTestUseDownload) std::cout << msgBuf << std::endl; msdo::download_status status2; - ASSERT_EQ(download.get_status(status).value(), 0); + ASSERT_EQ(download.get_status(status2).value(), 0); if (fPauseDownload) { ASSERT_EQ(download.pause().value(), 0); @@ -186,7 +186,7 @@ TEST_F(DownloadPropertyTests, SetCallbackTest) auto simpleDownload = g_MakeDownload(g_smallFileUrl, g_tmpFileName); int i= 0; - msdo::download_property_value callback = g_MakePropertyValue([&i](msdo::download& download, msdo::download_status& status) + msdo::download_property_value callback = g_MakePropertyValue([&i](msdo::download&, msdo::download_status& status) { char msgBuf[1024]; snprintf(msgBuf, sizeof(msgBuf), "Received status callback: %" PRIu64 "/%" PRIu64 ", 0x%x, 0x%x, %u", diff --git a/sdk-cpp/tests/download_tests_common.cpp b/sdk-cpp/tests/download_tests_common.cpp index f5873b5d..3e8fab55 100644 --- a/sdk-cpp/tests/download_tests_common.cpp +++ b/sdk-cpp/tests/download_tests_common.cpp @@ -3,10 +3,6 @@ #include "tests_common.h" -#ifdef DO_CLIENT_DOSVC -#include // HRESULT_FROM_WIN32, ERROR_FILE_NOT_FOUND -#endif - #include #include #include @@ -42,7 +38,7 @@ using namespace std::chrono_literals; // NOLINT(build/namespaces) #define DO_ERROR_FROM_SYSTEM_ERROR(x) (int32_t)(0xC0000000 | (FACILITY_DELIVERY_OPTIMIZATION << 16) | ((int32_t)(x) & 0x0000FFFF)) #endif -void WaitForDownloadCompletion(msdot::download& simpleDownload) +static void WaitForDownloadCompletion(msdot::download& simpleDownload) { msdo::download_status status = simpleDownload.get_status(); const auto endtime = std::chrono::steady_clock::now() + 5min; @@ -161,7 +157,7 @@ TEST_F(DownloadTests, SimpleDownloadTest_WithMalformedPath) catch (const msdod::exception& e) { #if defined(DO_INTERFACE_COM) - constexpr auto c_invalidDeviceName = (int)0x8007007b; + constexpr auto c_invalidDeviceName = static_cast(0x8007007b); std::array expectedErrors{ E_ACCESSDENIED, HTTP_E_STATUS_NOT_FOUND, c_invalidDeviceName }; // DO returns different errors on dev machine and pipeline agents (Win10/Win11?) ASSERT_TRUE(std::find(expectedErrors.begin(), expectedErrors.end(), e.error_code().value()) != expectedErrors.end()) @@ -185,7 +181,7 @@ TEST_F(DownloadTests, SimpleDownloadTest_With404UrlAndMalformedPath) catch (const msdod::exception& e) { #if defined(DO_INTERFACE_COM) - constexpr auto c_invalidDeviceName = (int)0x8007007b; + constexpr auto c_invalidDeviceName = static_cast(0x8007007b); std::array expectedErrors{ E_ACCESSDENIED, HTTP_E_STATUS_NOT_FOUND, c_invalidDeviceName }; // DO returns different errors on dev machine and pipeline agents (Win10/Win11?) ASSERT_TRUE(std::find(expectedErrors.begin(), expectedErrors.end(), e.error_code().value()) != expectedErrors.end()) @@ -588,7 +584,7 @@ TEST_F(DownloadTests, FileDeletionAfterPause) largeDownload->resume(); TestHelpers::WaitForState(*largeDownload, msdo::download_state::paused); status = largeDownload->get_status(); - ASSERT_EQ(status.error_code().value(), HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)); + ASSERT_EQ(status.error_code().value(), static_cast(0x80070002)); #endif } diff --git a/sdk-cpp/tests/test_helpers.h b/sdk-cpp/tests/test_helpers.h index b650178b..3736d115 100644 --- a/sdk-cpp/tests/test_helpers.h +++ b/sdk-cpp/tests/test_helpers.h @@ -16,7 +16,6 @@ namespace msdo = microsoft::deliveryoptimization; namespace msdod = microsoft::deliveryoptimization::details; -using namespace std::chrono_literals; // NOLINT(build/namespaces) namespace microsoft { @@ -174,13 +173,13 @@ class TestHelpers // On Windows, operations are async - there may be some delay setting a state internally static void WaitForState(microsoft::deliveryoptimization::test::download& download, msdo::download_state expectedState, - std::chrono::seconds waitTimeSecs = 10s) + std::chrono::seconds waitTimeSecs = std::chrono::seconds{10}) { msdo::download_status status = download.get_status(); const auto endtime = std::chrono::steady_clock::now() + waitTimeSecs; while ((status.state() != expectedState) && (std::chrono::steady_clock::now() < endtime)) { - std::this_thread::sleep_for(1s); + std::this_thread::sleep_for(std::chrono::seconds{1}); status = download.get_status(); std::cout << "Transferred " << status.bytes_transferred() << " / " << status.bytes_total() << "\n"; } From 151c58eadd2d9fc945bdb65612ec88266b9c5d20 Mon Sep 17 00:00:00 2001 From: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com> Date: Mon, 19 Sep 2022 12:29:19 -0700 Subject: [PATCH 04/11] Agent: Use flags not names of network interfaces (#138) * Network interface names can vary, leading to false 'no network' detections. Switching to flags as an alternative solution. * We should look into Network Manager dbus API in future if we need a more feature-rich and bullet-proof solution. --- client-lite/src/config/network_monitor.cpp | 35 +++++++++++----------- client-lite/src/config/network_monitor.h | 2 +- client-lite/src/download/download.cpp | 4 +-- client-lite/test/network_monitor_tests.cpp | 8 ++--- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/client-lite/src/config/network_monitor.cpp b/client-lite/src/config/network_monitor.cpp index bb626e45..34e71a50 100644 --- a/client-lite/src/config/network_monitor.cpp +++ b/client-lite/src/config/network_monitor.cpp @@ -6,11 +6,9 @@ #include #include +#include -// https://www.freedesktop.org/software/systemd/man/systemd.net-naming-scheme.html -static const char* g_publicIfNames[] = { "eth", "wlan", "en", "wl", "ww" }; - -bool NetworkMonitor::IsConnected() +bool NetworkMonitor::HasViableInterface() { struct ifaddrs* ifaddr; if (getifaddrs(&ifaddr) == -1) @@ -19,6 +17,10 @@ bool NetworkMonitor::IsConnected() return true; } + // Assume network connectivity is available if there is at least one network interface + // that has an IPv4/IPv6 address, is running and not a loopback interface. + // TODO(shishirb): Look into NetworkManager dbus API if needed (ability to distinguish among + // local/portal/internet connectivity, or in case of false detections with current logic). for (struct ifaddrs* ifa = ifaddr; ifa != nullptr; ifa = ifa->ifa_next) { if (ifa->ifa_addr == nullptr) @@ -32,23 +34,20 @@ bool NetworkMonitor::IsConnected() continue; } - // TODO(shishirb): Look for a better way to find the relevant adapter. Naming conventions can change. - for (auto ifname : g_publicIfNames) + if ((ifa->ifa_flags & IFF_RUNNING) + && !(ifa->ifa_flags & IFF_LOOPBACK)) { - auto foundPos = strcasestr(ifa->ifa_name, ifname); - if ((foundPos != nullptr) && (foundPos == ifa->ifa_name)) - { - DoLogInfo("Network connectivity detected. Interface: %s, address family: %d%s.", - ifa->ifa_name, family, - (family == AF_INET) ? " (AF_INET)" : - (family == AF_INET6) ? " (AF_INET6)" : ""); - freeifaddrs(ifaddr); - return true; - } + DoLogInfo("Viable network interface detected: %s, family: %d%s, flags: 0x%x.", + ifa->ifa_name, family, + (family == AF_INET) ? " (AF_INET)" : + (family == AF_INET6) ? " (AF_INET6)" : "", + ifa->ifa_flags); + freeifaddrs(ifaddr); + return true; } } - DoLogWarning("No network connectivity detected"); + DoLogWarning("No viable network interface"); freeifaddrs(ifaddr); return false; -} \ No newline at end of file +} diff --git a/client-lite/src/config/network_monitor.h b/client-lite/src/config/network_monitor.h index 507d26a9..56c04cdc 100644 --- a/client-lite/src/config/network_monitor.h +++ b/client-lite/src/config/network_monitor.h @@ -6,5 +6,5 @@ class NetworkMonitor { public: - static bool IsConnected(); + static bool HasViableInterface(); }; diff --git a/client-lite/src/download/download.cpp b/client-lite/src/download/download.cpp index 05b16668..a11926ec 100644 --- a/client-lite/src/download/download.cpp +++ b/client-lite/src/download/download.cpp @@ -357,7 +357,7 @@ void Download::_HandleTransientError(HRESULT hr) _status._Paused(S_OK, hr); _taskThread.Sched([this]() { - if (NetworkMonitor::IsConnected()) + if (NetworkMonitor::HasViableInterface()) { _ResumeAfterTransientError(); } @@ -693,7 +693,7 @@ HRESULT Download::OnComplete(HRESULT hrRequest, HRESULT hrCallback) _httpStatusCode = httpStatusCode; _responseHeaders = std::move(responseHeaders); - if (!NetworkMonitor::IsConnected()) + if (!NetworkMonitor::HasViableInterface()) { _HandleTransientError(DO_E_BLOCKED_BY_NO_NETWORK); return; diff --git a/client-lite/test/network_monitor_tests.cpp b/client-lite/test/network_monitor_tests.cpp index a0106722..2b043ce9 100644 --- a/client-lite/test/network_monitor_tests.cpp +++ b/client-lite/test/network_monitor_tests.cpp @@ -35,13 +35,13 @@ void NetworkMonitorTests::TearDown() // We can remove this test once we have it running as part of the E2E test suite. TEST_F(NetworkMonitorTests, DISABLED_VerifyNetworkReconnect) { - ASSERT_TRUE(NetworkMonitor::IsConnected()); + ASSERT_TRUE(NetworkMonitor::HasViableInterface()); _DisableNetwork(); - ASSERT_FALSE(NetworkMonitor::IsConnected()); + ASSERT_FALSE(NetworkMonitor::HasViableInterface()); _EnableNetwork(); - ASSERT_TRUE(NetworkMonitor::IsConnected()); + ASSERT_TRUE(NetworkMonitor::HasViableInterface()); } // Execute this test after manually changing network interface name. @@ -51,7 +51,7 @@ TEST_F(NetworkMonitorTests, DISABLED_VerifyNetworkReconnect) // Automating the name change is not feasible - requires reboot, and can make build agents go offline. TEST_F(NetworkMonitorTests, BasicNetworkConnected) { - ASSERT_TRUE(NetworkMonitor::IsConnected()); + ASSERT_TRUE(NetworkMonitor::HasViableInterface()); } void NetworkMonitorTests::_EnableNetwork() From 65e87319f5bc17b997b48d2eab55de768032df96 Mon Sep 17 00:00:00 2001 From: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com> Date: Mon, 19 Sep 2022 16:18:41 -0700 Subject: [PATCH 05/11] Plugin: No need to build lib-dohttp (#139) --- common/CMakeLists.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt index 285cc267..3be76e92 100644 --- a/common/CMakeLists.txt +++ b/common/CMakeLists.txt @@ -2,7 +2,9 @@ # Licensed under the MIT License. if (DO_PLATFORM_LINUX OR DO_PLATFORM_MAC) - add_subdirectory(lib-dohttp) + if (DO_INCLUDE_AGENT OR DO_INCLUDE_SDK) + add_subdirectory(lib-dohttp) + endif() endif() if (DO_BUILD_TESTS) From 0b09017006db3d402d64a65ae823a1d423b92055 Mon Sep 17 00:00:00 2001 From: andretoyama-msft <106272532+andretoyama-msft@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:39:48 -0300 Subject: [PATCH 06/11] fix/allow admin to override mcc config with empty string (#135) * fix/allow admin to override mcc config with empty string * fix else condition * improve code readability * add unit test for admin config empty string * Update mcc_manager.tests.cpp fix/remove blank space --- client-lite/src/config/config_manager.cpp | 6 +++--- client-lite/src/config/config_manager.h | 2 +- client-lite/src/config/mcc_manager.cpp | 10 ++++++++-- client-lite/test/mcc_manager.tests.cpp | 10 ++++++++++ 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/client-lite/src/config/config_manager.cpp b/client-lite/src/config/config_manager.cpp index 4ad937db..f1a044d9 100644 --- a/client-lite/src/config/config_manager.cpp +++ b/client-lite/src/config/config_manager.cpp @@ -43,10 +43,10 @@ boost::optional ConfigManager::CacheHostFallbackDelay() return returnValue; } -std::string ConfigManager::CacheHostServer() +boost::optional ConfigManager::CacheHostServer() { boost::optional cacheHostServer = _adminConfigs.Get(ConfigName_CacheHostServer); - return boost::get_optional_value_or(cacheHostServer, std::string{}); + return cacheHostServer; } std::string ConfigManager::IoTConnectionString() @@ -59,4 +59,4 @@ bool ConfigManager::RestControllerValidateRemoteAddr() { boost::optional validateRemoteAddr = _adminConfigs.Get(ConfigName_RestControllerValidateRemoteAddr); return boost::get_optional_value_or(validateRemoteAddr, g_RestControllerValidateRemoteAddrDefault); -} +} diff --git a/client-lite/src/config/config_manager.h b/client-lite/src/config/config_manager.h index 6b3cc89e..1f9c577e 100644 --- a/client-lite/src/config/config_manager.h +++ b/client-lite/src/config/config_manager.h @@ -15,7 +15,7 @@ class ConfigManager void RefreshAdminConfigs(); boost::optional CacheHostFallbackDelay(); - std::string CacheHostServer(); + boost::optional CacheHostServer(); std::string IoTConnectionString(); bool RestControllerValidateRemoteAddr(); diff --git a/client-lite/src/config/mcc_manager.cpp b/client-lite/src/config/mcc_manager.cpp index 79588904..81893845 100644 --- a/client-lite/src/config/mcc_manager.cpp +++ b/client-lite/src/config/mcc_manager.cpp @@ -50,8 +50,14 @@ boost::optional MCCManager::FallbackDelay() std::string MCCManager::GetHost() { - std::string mccHostName =_configManager.CacheHostServer(); - if (mccHostName.empty()) + boost::optional mccHostNameOpt =_configManager.CacheHostServer(); + std::string mccHostName; + + if (mccHostNameOpt.is_initialized()) + { + mccHostName = mccHostNameOpt.get(); + } + else { const std::string connString = _configManager.IoTConnectionString(); if (!connString.empty()) diff --git a/client-lite/test/mcc_manager.tests.cpp b/client-lite/test/mcc_manager.tests.cpp index db7b3d50..3769df46 100644 --- a/client-lite/test/mcc_manager.tests.cpp +++ b/client-lite/test/mcc_manager.tests.cpp @@ -121,6 +121,16 @@ TEST_F(MCCManagerTests, AdminConfigOverride) _VerifyExpectedCacheHost(TEST_MOCK_MCC_HOST); } +TEST_F(MCCManagerTests, AdminConfigEmptyString) +{ + SetIoTConnectionString("HostName=instance-company-iothub-ver.host.tld;DeviceId=user-dev-name;SharedAccessKey=abcdefghijklmnopqrstuvwxyzABCDE123456789012=;GatewayHostName=" TEST_MOCK_MCC_HOST); + SetDOCacheHostConfig(""); + _VerifyExpectedCacheHost(""); + + cppfs::remove(g_adminConfigFilePath); + _VerifyExpectedCacheHost(TEST_MOCK_MCC_HOST); +} + // Disabled tests: Azure lab MCC instance isn't responding quickly with 404. // Running it locally: ./deliveryoptimization-agent-tests --gtest_also_run_disabled_tests --gtest_filter=*DISABLED_Download404WithFallback -m TEST_F(MCCManagerTests, DISABLED_Download404WithFallback) From a4d5bd8f33e9edc9a3891fc29684884c450dd80c Mon Sep 17 00:00:00 2001 From: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com> Date: Tue, 27 Sep 2022 23:40:48 -0700 Subject: [PATCH 07/11] Agent: Fix debian package upgrade issue (#140) * During an upgrade, postrm script deletes the 'do' group causing 'adu' membership to be removed. * postrm script is now updated to perform cleanup only on remove and purge actions. postinst script already handled the case of existing user/group entities. * Also move systemctl reset-failed to postinst as the more appropriate location. If retaining in prerm, we will have to take a call on whether to call it only on upgrade/abort-upgrade/etc. --- client-lite/build/postinst.in.sh | 18 ++++++---- client-lite/build/postrm.in.sh | 59 +++++++++++++++++++++++--------- client-lite/build/prerm.in.sh | 4 +-- 3 files changed, 55 insertions(+), 26 deletions(-) diff --git a/client-lite/build/postinst.in.sh b/client-lite/build/postinst.in.sh index 7276b0f2..19135c61 100644 --- a/client-lite/build/postinst.in.sh +++ b/client-lite/build/postinst.in.sh @@ -14,15 +14,17 @@ svc_bin_path=@docs_svc_bin_path@ # Exit early to fail the install if any command here fails set -e -echo "Running post-install script for $svc_name" +echo "**** Running post-install script for $svc_name, args: $1 $2 ****" -if [ ! -f $svc_bin_path ]; then echo "docs binary cannot be found"; exit 1; fi +if [ ! -f $svc_bin_path ]; then echo "Agent binary cannot be found at $svc_bin_path"; exit 1; fi if ! getent group $do_group_name > /dev/null; then + echo "Creating group '$do_group_name'" addgroup --system $do_group_name fi if ! getent passwd $do_user_name > /dev/null; then + echo "Creating user '$do_user_name'" adduser --system $do_user_name --ingroup $do_group_name --shell /bin/false fi @@ -41,11 +43,11 @@ done configure_dir() { local dir_path="$1" echo "Configuring dir: $dir_path" - if [ ! -d $dir_path ]; then - mkdir $dir_path + if [ ! -d "$dir_path" ]; then + mkdir "$dir_path" fi - chgrp -R $do_group_name $dir_path - chown $do_user_name $dir_path + chgrp -R $do_group_name "$dir_path" + chown $do_user_name "$dir_path" } configure_dir "$config_path" @@ -72,6 +74,8 @@ echo "Service bin located at: $svc_bin_path" echo "Reloading systemd daemon list and enabling $svc_name" systemctl daemon-reload systemctl enable $svc_name +# Installed/upgraded daemon; remove from the failed services list. No-op if never failed earlier. +systemctl reset-failed $svc_name systemctl stop $svc_name > /dev/null # stop if already running systemctl start $svc_name -echo "Done!" +echo "**** Done! ****" diff --git a/client-lite/build/postrm.in.sh b/client-lite/build/postrm.in.sh index ddd42b2f..0e2bf973 100644 --- a/client-lite/build/postrm.in.sh +++ b/client-lite/build/postrm.in.sh @@ -11,29 +11,54 @@ run_path=@docs_svc_run_dir_path@ svc_name=@docs_svc_name@ svc_config_path=@docs_systemd_cfg_path@ -echo "Running post-removal script for $svc_name" +echo "**** Running post-removal script for $svc_name, args: $1 $2 ****" -systemctl reset-failed $svc_name # Remove ourselves from the failed services list. No-op if never failed earlier. +do_remove_daemon() { + echo "Removing systemd unit file: $svc_config_path" + rm $svc_config_path -echo "Removing systemd unit file: $svc_config_path" -rm $svc_config_path + echo "Reloading daemons" + systemctl daemon-reload +} -echo "Reloading daemons" -systemctl daemon-reload +do_remove_dirs() { + echo "Removing log directory: $log_path" + rm -rf $log_path -echo "Removing log directory: $log_path" -rm -rf $log_path + echo "Removing run directory: $run_path" + rm -rf $run_path +} -echo "Removing config directory: $config_path" -rm -rf $config_path +do_remove_user_and_group() { + echo "Removing user: ${do_user_name}" + userdel ${do_user_name} -echo "Removing run directory: $run_path" -rm -rf $run_path + echo "Removing group: ${do_group_name}" + groupdel ${do_group_name} +} -echo "Removing user: ${do_user_name}" -userdel ${do_user_name} +do_remove_tasks() { + do_remove_daemon + do_remove_dirs + do_remove_user_and_group +} -echo "Removing group: ${do_group_name}" -groupdel ${do_group_name} +case "$1" in + remove) + do_remove_tasks + ;; -echo "Done!" + purge) + do_remove_tasks + + echo "Removing config directory: $config_path" + rm -rf $config_path + ;; + + # upgrade: Removing user/group/directories during an upgrade is a mistake that can impact group membership, historical logs, etc. + # abort-install, abort-upgrade: This project does not use any package install path that requires handling these cases. + upgrade|abort-install|abort-upgrade) + ;; +esac + +echo "**** Done! ****" diff --git a/client-lite/build/prerm.in.sh b/client-lite/build/prerm.in.sh index 49847eb2..b405c45a 100644 --- a/client-lite/build/prerm.in.sh +++ b/client-lite/build/prerm.in.sh @@ -5,10 +5,10 @@ svc_name=@docs_svc_name@ -echo "Running pre-removal script for $svc_name" +echo "**** Running pre-removal script for $svc_name, args: $1 $2 ****" echo "Stopping and disabling service $svc_name" systemctl stop $svc_name systemctl disable $svc_name -echo "Done!" +echo "**** Done! ****" From 504832cd0d381cbb3b741c4f08454088b13f4489 Mon Sep 17 00:00:00 2001 From: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com> Date: Thu, 6 Oct 2022 09:23:43 -0700 Subject: [PATCH 08/11] Increment versions for early access (#141) * Agent, SDK: v0.7.1 * Plugin-Apt: v0.5.1 --- client-lite/CMakeLists.txt | 2 +- plugins/linux-apt/CMakeLists.txt | 2 +- sdk-cpp/CMakeLists.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client-lite/CMakeLists.txt b/client-lite/CMakeLists.txt index d1c27904..eb19ab84 100644 --- a/client-lite/CMakeLists.txt +++ b/client-lite/CMakeLists.txt @@ -5,7 +5,7 @@ if (NOT DOSVC_BIN_NAME) message (FATAL_ERROR "Agent daemon name not defined") endif () -project (${DOSVC_BIN_NAME} VERSION 0.7.0) +project (${DOSVC_BIN_NAME} VERSION 0.7.1) option (DO_PROXY_SUPPORT "Set DO_PROXY_SUPPORT to OFF to turn off proxy support for downloads and thus remove dependency on libproxy." ON) diff --git a/plugins/linux-apt/CMakeLists.txt b/plugins/linux-apt/CMakeLists.txt index de57ddb5..ff675b4a 100644 --- a/plugins/linux-apt/CMakeLists.txt +++ b/plugins/linux-apt/CMakeLists.txt @@ -5,7 +5,7 @@ if (NOT DO_PLUGIN_APT_BIN_NAME) message (FATAL_ERROR "APT plugin binary name not defined") endif () -project (${DO_PLUGIN_APT_BIN_NAME} VERSION 0.5.0) +project (${DO_PLUGIN_APT_BIN_NAME} VERSION 0.5.1) find_package(deliveryoptimization_sdk CONFIG REQUIRED) find_package(OpenSSL REQUIRED) diff --git a/sdk-cpp/CMakeLists.txt b/sdk-cpp/CMakeLists.txt index a451309c..886777ea 100644 --- a/sdk-cpp/CMakeLists.txt +++ b/sdk-cpp/CMakeLists.txt @@ -1,7 +1,7 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -project (deliveryoptimization_sdk VERSION 0.7.0) +project (deliveryoptimization_sdk VERSION 0.7.1) if (DO_PLATFORM_LINUX OR DO_PLATFORM_MAC) message("DO configured to use shared Boost libraries") From 4d3dad97a81643f19eb04fab9b3f9838b5f40c68 Mon Sep 17 00:00:00 2001 From: Shishir Bhat Date: Fri, 28 Oct 2022 07:30:50 -0700 Subject: [PATCH 09/11] Update README * Indicate plugin is for MCC usage and is in preview state. * Remove Debian9 from pipeline status badges (not supported anymore). --- README.md | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 5c0362a9..3e263d61 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,8 @@ through native C++ code. ## Plug-ins Add-on that enables APT downloads to go through Delivery Optimization Agent. -It is requires the SDK and Agent components. +It is a required component only on devices that must download APT packages via a Microsoft Connected Cache instance. +During install, it replaces itself as APT's HTTP(S) transport mechanism, thus receiving all APT downloads requests. ## Getting Started @@ -39,8 +40,8 @@ Run the appropriate bootstrapper depending on development machine platform: > cd build/bootstrap ``` -### Building DO client components -**NOTICE:** +### Building DO client components +**NOTICE:** **If you are modifying this project and distributing your own custom build, please modify the DO_BUILDER_IDENTIFIER cmake variable located in https://github.com/microsoft/do-client/blob/main/CMakeLists.txt** After setting up your development machine, navigate back into the project root @@ -49,7 +50,7 @@ After setting up your development machine, navigate back into the project root > cd ``` -We provide an easy-to-use python script for building our client components from the project root, you can inspect build.py for additional build flags +We provide an easy-to-use python script for building our client components from the project root, you can inspect build.py for additional build flags On debian-based systems, run this command to build the client and package it as a .deb file ```markdown @@ -71,7 +72,7 @@ Navigate to the build output directory for the agent and install the agent packa > sudo apt-get install ./deliveryoptimization-agent*.deb ``` -The sdk produces a runtime and development package, in this case you'll want to install both +The sdk produces a runtime and development package, in this case you'll want to install both Navigate to build output directory for the sdk and install both packages ```markdown @@ -92,7 +93,7 @@ At this point, you should have built and packaged all components There are a couple ways for you to install the DO client components -1. If you have built the component into a debian package, you can simply find the debian package and install like detailed above. +1. If you have built the component into a debian package, you can simply find the debian package and install like detailed above. This will handle installing to the appropriate paths, and also the necessary setup of DO user/group permissions needed for DO-agent. ```markdown @@ -104,19 +105,19 @@ This will handle installing to the appropriate paths, and also the necessary set > sudo apt get install ./deliveryoptimization-plugin-apt*.deb ``` -2. If you build and install using cmake, or through some other custom means, be sure to setup the DO user/groups correctly in your installation. +2. If you build and install using cmake, or through some other custom means, be sure to setup the DO user/groups correctly in your installation. You can reference this [script](https://github.com/microsoft/do-client/blob/main/client-lite/build/postinst.in.sh) to see how to setup the DO user/group and install DO as a daemon. ### Testing DO Client components -As guidance, please ensure proper code coverage for project contributions +As guidance, please ensure proper code coverage for project contributions Unit tests for the agent and sdk are produced as a part of the above build command, you can find them in the build output directory ```markdown > cd /tmp/build-deliveryoptimization-agent/linux-debug/client-lite/test ``` -Our tests utilize the [GTest](https://github.com/google/googletest) unit testing framework, which supports test filtering via command line +Our tests utilize the [GTest](https://github.com/google/googletest) unit testing framework, which supports test filtering via command line You can run all agent tests by running ```markdown @@ -134,7 +135,7 @@ The test executable for the SDK is located in the sdk build output as well > cd /tmp/build-deliveryoptimization-sdk/linux-debug/sdk-cpp/tests ``` -The sdk tests expect a running do-agent, you can either manually run the agent executable from its build output or install the agent package as you may have done while building the plugin +The sdk tests expect a running do-agent, you can either manually run the agent executable from its build output or install the agent package as you may have done while building the plugin You can run the sdk tests just like the agent tests ```markdown @@ -149,10 +150,10 @@ And filter them similarly ## Support -This repository is currently in a **Public Preview** state. During this phase, all DO components -found in this repo will be supported for 90 days beyond the release date of a new release. At -the end of the 90 day window, we will not guarantee support for the previous version. Please plan -to migrate to the new DO components within that 90-day window to avoid any disruptions. +The APT plugin component is currently in a **Public Preview** state. During this phase, it will be +supported for 90 days beyond the release date of a new release. At the end of the 90 day window, +we will not guarantee support for the previous version. Please plan to migrate to a newer release +within that 90-day window to avoid any disruptions. ## Filing a Bug @@ -175,12 +176,6 @@ tracked appropriately. | x86-64 | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20Simple%20Client%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=25&branchName=main) | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20CPP-SDK%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=33&branchName=main) | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20Plugins%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=31&branchName=main) | | arm64 | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20Simple%20Client%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=25&branchName=main) | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20CPP-SDK%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=33&branchName=main) | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20Plugins%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=31&branchName=main) | -#### Debian 9 - -| Architecture | Agent | SDK | Plugin | -|-----|--------|-----|--------| -| arm32 | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20Simple%20Client%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=25&branchName=main) | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20CPP-SDK%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=33&branchName=main) | [![Build Status](https://deliveryoptimization.visualstudio.com/client/_apis/build/status/DO%20Plugins%20ARM%20Build?branchName=main)](https://deliveryoptimization.visualstudio.com/client/_build/latest?definitionId=31&branchName=main) | - #### Debian 10 | Architecture | Agent | SDK | Plugin | From 17043f1367cde658f9e5f12422a6170533e152cf Mon Sep 17 00:00:00 2001 From: Shishir Bhat Date: Fri, 28 Oct 2022 07:36:56 -0700 Subject: [PATCH 10/11] Remove Debian9 from build pipelines * OS version no longer supported --- .../build/linux/du/docker/doclient-lite-docker.yml | 6 ------ azure-pipelines/build/linux/du/docker/dopapt-docker.yml | 6 ------ azure-pipelines/build/linux/du/docker/dosdkcpp-docker.yml | 6 ------ azure-pipelines/build/linux/du/templates/do-docker-jobs.yml | 2 +- .../build/linux/du/templates/doclient-lite-docker-steps.yml | 2 +- .../build/linux/du/templates/dopapt-docker-steps.yml | 2 +- .../build/linux/du/templates/dosdkcpp-docker-steps.yml | 2 +- azure-pipelines/publishing/github-release.yml | 4 ---- .../publishing/templates/release-docker-build-steps.yml | 6 +++--- .../publishing/templates/release-native-build-steps.yml | 6 +++--- 10 files changed, 10 insertions(+), 32 deletions(-) diff --git a/azure-pipelines/build/linux/du/docker/doclient-lite-docker.yml b/azure-pipelines/build/linux/du/docker/doclient-lite-docker.yml index 766a1415..52c65b73 100644 --- a/azure-pipelines/build/linux/du/docker/doclient-lite-docker.yml +++ b/azure-pipelines/build/linux/du/docker/doclient-lite-docker.yml @@ -25,12 +25,6 @@ pool: demands: ImageOverride -equals do-adu-build-$(imageVersion) jobs: -- template: ../templates/do-docker-jobs.yml - parameters: - targetOsArch: 'debian9_arm32' - imageVersion: '0.8.0' #hard coded version because debian9 support will soon be removed - stepsTemplate: 'doclient-lite-docker-steps.yml' - - template: ../templates/do-docker-jobs.yml parameters: targetOsArch: 'debian10_arm32' diff --git a/azure-pipelines/build/linux/du/docker/dopapt-docker.yml b/azure-pipelines/build/linux/du/docker/dopapt-docker.yml index 81740832..bbda6c76 100644 --- a/azure-pipelines/build/linux/du/docker/dopapt-docker.yml +++ b/azure-pipelines/build/linux/du/docker/dopapt-docker.yml @@ -25,12 +25,6 @@ pool: demands: ImageOverride -equals do-adu-build-$(imageVersion) jobs: -- template: ../templates/do-docker-jobs.yml - parameters: - targetOsArch: 'debian9_arm32' - imageVersion: '0.8.0' #hard coded version because debian9 support will soon be removed - stepsTemplate: 'dopapt-docker-steps.yml' - - template: ../templates/do-docker-jobs.yml parameters: targetOsArch: 'debian10_arm32' diff --git a/azure-pipelines/build/linux/du/docker/dosdkcpp-docker.yml b/azure-pipelines/build/linux/du/docker/dosdkcpp-docker.yml index b95851f7..8e4349ec 100644 --- a/azure-pipelines/build/linux/du/docker/dosdkcpp-docker.yml +++ b/azure-pipelines/build/linux/du/docker/dosdkcpp-docker.yml @@ -43,12 +43,6 @@ jobs: imageVersion: ${{variables.containerImageVersion}} stepsTemplate: 'dosdkcpp-docker-steps.yml' -- template: ../templates/do-docker-jobs.yml - parameters: - targetOsArch: 'debian9_arm32' - imageVersion: ${{variables.containerImageVersion}} - stepsTemplate: 'dosdkcpp-docker-steps.yml' - - template: ../templates/do-docker-jobs.yml parameters: targetOsArch: 'ubuntu1804_arm64' diff --git a/azure-pipelines/build/linux/du/templates/do-docker-jobs.yml b/azure-pipelines/build/linux/du/templates/do-docker-jobs.yml index dbf9f280..7e087058 100644 --- a/azure-pipelines/build/linux/du/templates/do-docker-jobs.yml +++ b/azure-pipelines/build/linux/du/templates/do-docker-jobs.yml @@ -2,7 +2,7 @@ # Consume this jobs template in a pipeline yaml by passing in parameter values. parameters: -- name: targetOsArch # example: debian9_arm32 +- name: targetOsArch # example: ubuntu2004_arm64 type: string - name: imageVersion type: string diff --git a/azure-pipelines/build/linux/du/templates/doclient-lite-docker-steps.yml b/azure-pipelines/build/linux/du/templates/doclient-lite-docker-steps.yml index 59165429..f03bf8c1 100644 --- a/azure-pipelines/build/linux/du/templates/doclient-lite-docker-steps.yml +++ b/azure-pipelines/build/linux/du/templates/doclient-lite-docker-steps.yml @@ -2,7 +2,7 @@ # Consume this steps template in one or more jobs by passing in parameter values. parameters: -- name: targetOsArch # example: debian9_arm32 +- name: targetOsArch # example: ubuntu2004_arm64 type: string - name: imageVersion type: string diff --git a/azure-pipelines/build/linux/du/templates/dopapt-docker-steps.yml b/azure-pipelines/build/linux/du/templates/dopapt-docker-steps.yml index 4461a15b..be74d4a3 100644 --- a/azure-pipelines/build/linux/du/templates/dopapt-docker-steps.yml +++ b/azure-pipelines/build/linux/du/templates/dopapt-docker-steps.yml @@ -2,7 +2,7 @@ # Consume this steps template in one or more jobs by passing in parameter values. parameters: -- name: targetOsArch # example: debian9_arm32 +- name: targetOsArch # example: ubuntu2004_arm64 type: string - name: imageVersion type: string diff --git a/azure-pipelines/build/linux/du/templates/dosdkcpp-docker-steps.yml b/azure-pipelines/build/linux/du/templates/dosdkcpp-docker-steps.yml index 15098274..80f4f309 100644 --- a/azure-pipelines/build/linux/du/templates/dosdkcpp-docker-steps.yml +++ b/azure-pipelines/build/linux/du/templates/dosdkcpp-docker-steps.yml @@ -2,7 +2,7 @@ # Consume this steps template in one or more jobs by passing in parameter values. parameters: -- name: targetOsArch # example: debian9_arm32 +- name: targetOsArch # example: ubuntu2004_arm64 type: string - name: imageVersion type: string diff --git a/azure-pipelines/publishing/github-release.yml b/azure-pipelines/publishing/github-release.yml index d177017f..03d0eb39 100644 --- a/azure-pipelines/publishing/github-release.yml +++ b/azure-pipelines/publishing/github-release.yml @@ -60,10 +60,6 @@ stages: parameters: targetOsArch: 'ubuntu2004_arm64' imageVersion: ${{variables.containerImageVersion}} - - template: templates/release-docker-build-steps.yml - parameters: - targetOsArch: 'debian9_arm32' - imageVersion: '0.8.0' #hard coded version because debian9 support will soon be removed - template: templates/release-docker-build-steps.yml parameters: targetOsArch: 'debian10_arm32' diff --git a/azure-pipelines/publishing/templates/release-docker-build-steps.yml b/azure-pipelines/publishing/templates/release-docker-build-steps.yml index b29102ca..77d3a86b 100644 --- a/azure-pipelines/publishing/templates/release-docker-build-steps.yml +++ b/azure-pipelines/publishing/templates/release-docker-build-steps.yml @@ -2,7 +2,7 @@ # Consume this steps template in one or more jobs by passing in parameter values. parameters: -- name: targetOsArch # example: debian9_arm32 +- name: targetOsArch # example: ubuntu2004_arm64 type: string - name: imageVersion type: string @@ -60,14 +60,14 @@ jobs: displayName: 'Copy plugin .deb file' - task: DeleteFiles@1 - inputs: + inputs: SourceFolder: $(Build.ArtifactStagingDirectory) Contents: | **/* displayName: 'Clean build folder before creating tar file folder' - task: ArchiveFiles@2 - inputs: + inputs: rootFolderOrFile: /tmp/${{parameters.targetOsArch}} includeRootFolder: False archiveType: tar diff --git a/azure-pipelines/publishing/templates/release-native-build-steps.yml b/azure-pipelines/publishing/templates/release-native-build-steps.yml index c3898f8c..9dcf54de 100644 --- a/azure-pipelines/publishing/templates/release-native-build-steps.yml +++ b/azure-pipelines/publishing/templates/release-native-build-steps.yml @@ -1,5 +1,5 @@ parameters: -- name: targetOsArch # example: debian9_arm32 +- name: targetOsArch # example: ubuntu2004_arm64 type: string jobs: @@ -53,14 +53,14 @@ jobs: displayName: 'Copy plugin .deb file' - task: DeleteFiles@1 - inputs: + inputs: SourceFolder: $(Build.ArtifactStagingDirectory) Contents: | **/* displayName: 'Clean build folder before creating tar file folder' - task: ArchiveFiles@2 - inputs: + inputs: rootFolderOrFile: /tmp/${{parameters.targetOsArch}} includeRootFolder: False archiveType: tar From a5dfc820fbdac30f6b7725e005fda6ede6272685 Mon Sep 17 00:00:00 2001 From: Shishir Bhat Date: Fri, 28 Oct 2022 07:40:22 -0700 Subject: [PATCH 11/11] Increment versions for GA * Agent, SDK: v1.0.0 --- client-lite/CMakeLists.txt | 2 +- sdk-cpp/CMakeLists.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/client-lite/CMakeLists.txt b/client-lite/CMakeLists.txt index eb19ab84..621f81f4 100644 --- a/client-lite/CMakeLists.txt +++ b/client-lite/CMakeLists.txt @@ -5,7 +5,7 @@ if (NOT DOSVC_BIN_NAME) message (FATAL_ERROR "Agent daemon name not defined") endif () -project (${DOSVC_BIN_NAME} VERSION 0.7.1) +project (${DOSVC_BIN_NAME} VERSION 1.0.0) option (DO_PROXY_SUPPORT "Set DO_PROXY_SUPPORT to OFF to turn off proxy support for downloads and thus remove dependency on libproxy." ON) diff --git a/sdk-cpp/CMakeLists.txt b/sdk-cpp/CMakeLists.txt index 886777ea..f43227d5 100644 --- a/sdk-cpp/CMakeLists.txt +++ b/sdk-cpp/CMakeLists.txt @@ -1,7 +1,7 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -project (deliveryoptimization_sdk VERSION 0.7.1) +project (deliveryoptimization_sdk VERSION 1.0.0) if (DO_PLATFORM_LINUX OR DO_PLATFORM_MAC) message("DO configured to use shared Boost libraries")