From 26247671d4cd79b2fc42856ce96ce9fc5a5ca736 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Mon, 1 Feb 2021 10:19:18 +0100 Subject: [PATCH 1/7] when compiling pyc, properly chcp --- src/link.cpp | 167 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 103 insertions(+), 64 deletions(-) diff --git a/src/link.cpp b/src/link.cpp index 1fc5f1897..0168dc727 100644 --- a/src/link.cpp +++ b/src/link.cpp @@ -463,6 +463,43 @@ namespace mamba return tf; } + auto prepare_wrapped_call(const fs::path& prefix, const std::vector& cmd) + { + std::vector command_args; + std::unique_ptr script_file; + + if (on_win) + { + ensure_comspec_set(); + std::string comspec = env::get("COMSPEC"); + if (comspec.size() == 0) + { + throw std::runtime_error( + concat("Failed to run script: COMSPEC not set in env vars.")); + } + + script_file = wrap_call( + Context::instance().root_prefix, prefix, Context::instance().dev, false, cmd); + + command_args = { comspec, "/d", "/c", script_file->path() }; + } + else + { + // shell_path = 'sh' if 'bsd' in sys.platform else 'bash' + fs::path shell_path = env::which("bash"); + if (shell_path.empty()) + { + shell_path = env::which("sh"); + } + + script_file = wrap_call( + Context::instance().root_prefix, prefix, Context::instance().dev, false, cmd); + command_args.push_back(shell_path); + command_args.push_back(script_file->path()); + } + return std::make_tuple(command_args, std::move(script_file)); + } + /* call the post-link or pre-unlink script and return true / false on success / failure @@ -473,13 +510,17 @@ namespace mamba const std::string& env_prefix = "", bool activate = false) { -#ifdef _WIN32 - auto path = prefix / get_bin_directory_short_path() - / concat(".", pkg_info.name, "-", action, ".bat"); -#else - auto path = prefix / get_bin_directory_short_path() - / concat(".", pkg_info.name, "-", action, ".sh"); -#endif + fs::path path; + if (on_win) + { + path = prefix / get_bin_directory_short_path() + / concat(".", pkg_info.name, "-", action, ".bat"); + } + else + { + path = prefix / get_bin_directory_short_path() + / concat(".", pkg_info.name, "-", action, ".sh"); + } if (!fs::exists(path)) { @@ -500,56 +541,60 @@ namespace mamba std::vector command_args; std::unique_ptr script_file; -#ifdef _WIN32 - ensure_comspec_set(); - std::string comspec = env::get("COMSPEC"); - if (comspec.size() == 0) + if (on_win) { - LOG_ERROR << "Failed to run " << action << " for " << pkg_info.name - << " due to COMSPEC not set in env vars."; - return false; + ensure_comspec_set(); + std::string comspec = env::get("COMSPEC"); + if (comspec.size() == 0) + { + LOG_ERROR << "Failed to run " << action << " for " << pkg_info.name + << " due to COMSPEC not set in env vars."; + return false; + } + + if (activate) + { + script_file = wrap_call(Context::instance().root_prefix, + prefix, + Context::instance().dev, + false, + { "@CALL", path }); + + command_args = { comspec, "/d", "/c", script_file->path() }; + } + else + { + command_args = { comspec, "/d", "/c", path }; + } } - if (activate) - { - script_file = wrap_call(Context::instance().root_prefix, - prefix, - Context::instance().dev, - false, - { "@CALL", path }); - - command_args = { comspec, "/d", "/c", script_file->path() }; - } else { - command_args = { comspec, "/d", "/c", path }; - } -#else - // shell_path = 'sh' if 'bsd' in sys.platform else 'bash' - fs::path shell_path = env::which("bash"); - if (shell_path.empty()) - { - shell_path = env::which("sh"); - } + // shell_path = 'sh' if 'bsd' in sys.platform else 'bash' + fs::path shell_path = env::which("bash"); + if (shell_path.empty()) + { + shell_path = env::which("sh"); + } - if (activate) - { - // std::string caller - script_file = wrap_call(Context::instance().root_prefix, - prefix, - Context::instance().dev, - false, - { ".", path }); - command_args.push_back(shell_path); - command_args.push_back(script_file->path()); + if (activate) + { + // std::string caller + script_file = wrap_call(Context::instance().root_prefix, + prefix, + Context::instance().dev, + false, + { ".", path }); + command_args.push_back(shell_path); + command_args.push_back(script_file->path()); + } + else + { + command_args.push_back(shell_path); + command_args.push_back("-x"); + command_args.push_back(path); + } } - else - { - command_args.push_back(shell_path); - command_args.push_back("-x"); - command_args.push_back(path); - } -#endif envmap["ROOT_PREFIX"] = Context::instance().root_prefix; envmap["PREFIX"] = env_prefix.size() ? env_prefix : std::string(prefix); @@ -875,9 +920,6 @@ namespace mamba } all_py_files_f.close(); - // TODO use the real python file here?! - LOG_DEBUG << "Running " << m_context->target_prefix / m_context->python_path - << " -Wi -m compileall -q -l -i " << all_py_files.path() << std::endl; std::vector command = { m_context->target_prefix / m_context->python_path, "-Wi", "-m", @@ -901,18 +943,15 @@ namespace mamba std::string cwd = m_context->target_prefix; options.working_directory = cwd.c_str(); - std::map envmap = env::copy(); -#ifdef _WIN32 - CmdExeActivator activator; -#else - PosixActivator activator; + auto [wrapped_command, script_file] + = prepare_wrapped_call(m_context->target_prefix, command); + LOG_INFO << "Running wrapped python compilation command " << join(" ", command); +#ifndef NDEBUG + std::ifstream ix(script_file->path()); + LOG_DEBUG << "Wrapped activation:\n" << ix.rdbuf() << "\n"; + ix.close(); #endif - std::string current_env_path = activator.add_prefix_to_path(m_context->target_prefix, 0); - envmap["PATH"] = current_env_path; - options.env.behavior = reproc::env::empty; - options.env.extra = envmap; - - auto [_, ec] = reproc::run(command, options); + auto [_, ec] = reproc::run(wrapped_command, options); if (ec) { From 23adcfeacd1b7155e217e59b1f2d24cb8510ee80 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Mon, 1 Feb 2021 18:38:23 +0100 Subject: [PATCH 2/7] add more cache checks --- include/mamba/thread_utils.hpp | 9 ++-- include/mamba/transaction.hpp | 16 +++++++ src/micromamba/main.cpp | 5 ++- src/thread_utils.cpp | 11 ++++- src/transaction.cpp | 79 ++++++++++++++++++++++++++++------ 5 files changed, 99 insertions(+), 21 deletions(-) diff --git a/include/mamba/thread_utils.hpp b/include/mamba/thread_utils.hpp index 3dc8d1b22..98dd82bba 100644 --- a/include/mamba/thread_utils.hpp +++ b/include/mamba/thread_utils.hpp @@ -50,7 +50,7 @@ namespace mamba // Must be called by the cleaning thread to ensure // it won't free ressources that could be required // by threads still active. - void wait_before_cleaning(); + void wait_for_all_threads(); void notify_cleanup(); // Should be called by the main thread to ensure @@ -145,7 +145,7 @@ namespace mamba m_cleanup_function = std::bind(std::forward(func), std::forward(args)...); std::signal(SIGINT, [](int) { set_sig_interrupted(); - wait_before_cleaning(); + wait_for_all_threads(); m_cleanup_function(); }); } @@ -162,8 +162,11 @@ namespace mamba wait_for_signal(); if (m_interrupt.load()) { + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, nullptr); + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, nullptr); + set_sig_interrupted(); - wait_before_cleaning(); + wait_for_all_threads(); f(); notify_cleanup(); } diff --git a/include/mamba/transaction.hpp b/include/mamba/transaction.hpp index 7fc05b091..3c12f1764 100644 --- a/include/mamba/transaction.hpp +++ b/include/mamba/transaction.hpp @@ -50,8 +50,23 @@ namespace mamba bool finalize_callback(); bool finished(); bool validate_extract(); + auto validation_result() const; + void clear_cache() const; + DownloadTarget* target(const fs::path& cache_path, MultiPackageCache& cache); + enum VALIDATION_RESULT + { + UNDEFINED = 0, + VALID = 1, + SHA256_ERROR, + MD5SUM_ERROR, + SIZE_ERROR, + EXTRACT_ERROR + }; + + std::exception m_decompress_exception; + private: bool m_finished; PackageInfo m_package_info; @@ -67,6 +82,7 @@ namespace mamba std::future m_extract_future; + VALIDATION_RESULT m_validation_result = VALIDATION_RESULT::UNDEFINED; static std::mutex extract_mutex; }; diff --git a/src/micromamba/main.cpp b/src/micromamba/main.cpp index 9a1d606b6..c03f70ba0 100644 --- a/src/micromamba/main.cpp +++ b/src/micromamba/main.cpp @@ -792,7 +792,6 @@ download_explicit(const std::vector& pkgs) std::vector> targets; MultiDownloadTarget multi_dl; MultiPackageCache pkg_cache({ Context::instance().root_prefix / "pkgs" }); - Console::instance().init_multi_progress(); for (auto& pkg : pkgs) { @@ -1240,9 +1239,11 @@ main(int argc, char** argv) { CLI11_PARSE(app, argc, argv); } - catch (std::exception& e) + catch (const std::exception& e) { LOG_ERROR << e.what(); + std::raise(SIGTERM); + std::cout << "Exiting." << std::endl; exit(1); } diff --git a/src/thread_utils.cpp b/src/thread_utils.cpp index 7fbefa013..a36f9ae85 100644 --- a/src/thread_utils.cpp +++ b/src/thread_utils.cpp @@ -78,7 +78,7 @@ namespace mamba return thread_count; } - void wait_before_cleaning() + void wait_for_all_threads() { std::unique_lock lk(clean_mutex); clean_var.wait(lk, []() { return thread_count == 0; }); @@ -98,11 +98,15 @@ namespace mamba namespace { - std::thread::native_handle_type cleanup_id; + std::thread::native_handle_type cleanup_id = 0; } void register_cleaning_thread_id(std::thread::native_handle_type id) { + if (cleanup_id) + { + pthread_cancel(cleanup_id); + } cleanup_id = id; } @@ -160,10 +164,13 @@ namespace mamba if (is_sig_interrupted()) { wait_for_cleanup(); + std::cout << "Cleanup done." << std::endl; } m_interrupt.store(false); pthread_sigmask(SIG_UNBLOCK, &sigset, nullptr); set_default_signal_handler(); + pthread_cancel(get_cleaning_thread_id()); + register_cleaning_thread_id(0); } void interruption_guard::block_signals() const diff --git a/src/transaction.cpp b/src/transaction.cpp index 3c49c3afb..64a3d6a9c 100644 --- a/src/transaction.cpp +++ b/src/transaction.cpp @@ -79,29 +79,39 @@ namespace mamba bool PackageDownloadExtractTarget::validate_extract() { // Validation + m_validation_result = VALIDATION_RESULT::VALID; if (m_expected_size && size_t(m_target->downloaded_size) != m_expected_size) { - LOG_ERROR << "File not valid: file size doesn't match expectation " << m_tarball_path; - throw std::runtime_error("File not valid: file size doesn't match expectation (" - + std::string(m_tarball_path) + ")"); + LOG_ERROR << "File not valid: file size doesn't match expectation " << m_tarball_path + << "\nExpected: " << m_expected_size << "\n"; + m_progress_proxy.mark_as_completed("File size validation error."); + m_validation_result = SIZE_ERROR; } interruption_point(); if (!m_sha256.empty() && !validate::sha256(m_tarball_path, m_sha256)) { - LOG_ERROR << "File not valid: SHA256 sum doesn't match expectation " << m_tarball_path; - throw std::runtime_error("File not valid: SHA256 sum doesn't match expectation (" - + std::string(m_tarball_path) + ")"); + m_validation_result = SHA256_ERROR; + m_progress_proxy.mark_as_completed("SHA256 sum validation error."); + LOG_ERROR << "File not valid: SHA256 sum doesn't match expectation " << m_tarball_path + << "\nExpected: " << m_sha256 << "\n"; } else { if (!m_md5.empty() && !validate::md5(m_tarball_path, m_md5)) { - LOG_ERROR << "File not valid: MD5 sum doesn't match expectation " << m_tarball_path; - throw std::runtime_error("File not valid: MD5 sum doesn't match expectation (" - + std::string(m_tarball_path) + ")"); + m_validation_result = MD5SUM_ERROR; + m_progress_proxy.mark_as_completed("MD5 sum validation error."); + LOG_ERROR << "File not valid: MD5 sum doesn't match expectation " << m_tarball_path + << "\nExpected: " << m_md5 << "\n"; } } + if (m_validation_result != VALIDATION_RESULT::VALID) + { + // abort here, but set finished to true + m_finished = true; + return true; + } interruption_point(); LOG_INFO << "Waiting for decompression " << m_tarball_path; @@ -112,10 +122,22 @@ namespace mamba interruption_point(); m_progress_proxy.set_postfix("Decompressing..."); LOG_INFO << "Decompressing " << m_tarball_path; - auto extract_path = extract(m_tarball_path); - LOG_INFO << "Extracted to " << extract_path; - write_repodata_record(extract_path); - add_url(); + try + { + auto extract_path = extract(m_tarball_path); + LOG_INFO << "Extracted to " << extract_path; + write_repodata_record(extract_path); + add_url(); + } + catch (std::exception& e) + { + LOG_ERROR << "Error when extracting package: " << e.what(); + m_decompress_exception = e; + m_validation_result = VALIDATION_RESULT::EXTRACT_ERROR; + m_finished = true; + m_progress_proxy.mark_as_completed("Extraction error"); + return m_finished; + } } interruption_point(); @@ -152,6 +174,21 @@ namespace mamba return m_target == nullptr ? true : m_finished; } + auto PackageDownloadExtractTarget::validation_result() const + { + return m_validation_result; + } + + void PackageDownloadExtractTarget::clear_cache() const + { + fs::remove_all(m_tarball_path); + fs::path dest_dir = strip_package_extension(m_tarball_path); + if (fs::exists(dest_dir)) + { + fs::remove_all(dest_dir); + } + } + // todo remove cache from this interface DownloadTarget* PackageDownloadExtractTarget::target(const fs::path& cache_path, MultiPackageCache& cache) @@ -166,6 +203,7 @@ namespace mamba if (valid && !dest_dir_exists) { // TODO add extract job here + // finalize_callback(); } // tarball can be removed, it's fine if only the correct dest dir exists @@ -663,6 +701,7 @@ namespace mamba interruption_guard g([]() { Console::instance().init_multi_progress(); }); bool downloaded = multi_dl.download(true); + bool all_valid = true; if (!downloaded) { @@ -688,7 +727,19 @@ namespace mamba std::this_thread::sleep_for(std::chrono::milliseconds(100)); } - return !is_sig_interrupted() && downloaded; + for (const auto& t : targets) + { + if (t->validation_result() != PackageDownloadExtractTarget::VALIDATION_RESULT::VALID + && t->validation_result() + != PackageDownloadExtractTarget::VALIDATION_RESULT::UNDEFINED) + { + t->clear_cache(); + all_valid = false; + throw std::runtime_error("Found incorrect download. Aborting"); + } + } + + return !is_sig_interrupted() && downloaded && all_valid; } bool MTransaction::empty() From 3affbcf836949f360cdffcabc3f799de54e2cbba Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Mon, 1 Feb 2021 19:33:59 +0100 Subject: [PATCH 3/7] conditional pthread_cancel --- src/thread_utils.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/thread_utils.cpp b/src/thread_utils.cpp index a36f9ae85..b5cdc4328 100644 --- a/src/thread_utils.cpp +++ b/src/thread_utils.cpp @@ -103,10 +103,12 @@ namespace mamba void register_cleaning_thread_id(std::thread::native_handle_type id) { +#ifndef _WIN32 if (cleanup_id) { pthread_cancel(cleanup_id); } +#endif cleanup_id = id; } From 02371f3223dd3761ceb033a11aea2ea7ce75f13a Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Mon, 1 Feb 2021 20:16:27 +0100 Subject: [PATCH 4/7] remove debug output --- src/link.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/link.cpp b/src/link.cpp index 0168dc727..dd1513b4d 100644 --- a/src/link.cpp +++ b/src/link.cpp @@ -946,11 +946,6 @@ namespace mamba auto [wrapped_command, script_file] = prepare_wrapped_call(m_context->target_prefix, command); LOG_INFO << "Running wrapped python compilation command " << join(" ", command); -#ifndef NDEBUG - std::ifstream ix(script_file->path()); - LOG_DEBUG << "Wrapped activation:\n" << ix.rdbuf() << "\n"; - ix.close(); -#endif auto [_, ec] = reproc::run(wrapped_command, options); if (ec) From c41339bd1656735c493e8877b2fda2f899d73070 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Tue, 2 Feb 2021 09:25:28 +0100 Subject: [PATCH 5/7] turn log warning to log info --- src/link.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/link.cpp b/src/link.cpp index dd1513b4d..4159d58a2 100644 --- a/src/link.cpp +++ b/src/link.cpp @@ -968,11 +968,11 @@ namespace mamba { LOG_INFO << "Executing install for " << m_source; nlohmann::json index_json, out_json; - LOG_WARNING << "Opening: " << m_source / "info" / "paths.json"; + LOG_INFO << "Opening: " << m_source / "info" / "paths.json"; auto paths_data = read_paths(m_source); - LOG_WARNING << "Opening: " << m_source / "info" / "repodata_record.json"; + LOG_INFO << "Opening: " << m_source / "info" / "repodata_record.json"; std::ifstream repodata_f(m_source / "info" / "repodata_record.json"); repodata_f >> index_json; From 8dfcd7cd46d9d043647e2f75d3029d12f6b009f6 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Tue, 2 Feb 2021 09:28:54 +0100 Subject: [PATCH 6/7] call pthread_cancelstate and type at the right place' --- include/mamba/thread_utils.hpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/include/mamba/thread_utils.hpp b/include/mamba/thread_utils.hpp index 98dd82bba..480aa8c2b 100644 --- a/include/mamba/thread_utils.hpp +++ b/include/mamba/thread_utils.hpp @@ -159,12 +159,13 @@ namespace mamba block_signals(); auto f = std::bind(std::forward(func), std::forward(args)...); std::thread victor_the_cleaner([f, this]() { + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, nullptr); + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, nullptr); + wait_for_signal(); + if (m_interrupt.load()) { - pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, nullptr); - pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, nullptr); - set_sig_interrupted(); wait_for_all_threads(); f(); From f96876cd0e27f29854218e991616d44776523c67 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Tue, 2 Feb 2021 09:43:37 +0100 Subject: [PATCH 7/7] first extract into temporary dir, then move to final pkgs cache --- src/package_handling.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/package_handling.cpp b/src/package_handling.cpp index 9e4e19d64..9090bebfd 100644 --- a/src/package_handling.cpp +++ b/src/package_handling.cpp @@ -371,22 +371,24 @@ namespace mamba fs::path extract(const fs::path& file) { std::string dest_dir = file; + TemporaryDirectory temp_extract_dir; + if (ends_with(dest_dir, ".tar.bz2")) { dest_dir = dest_dir.substr(0, dest_dir.size() - 8); - extract_archive(file, dest_dir); - return dest_dir; + extract_archive(file, temp_extract_dir.path()); } else if (ends_with(dest_dir, ".conda")) { dest_dir = dest_dir.substr(0, dest_dir.size() - 6); - extract_conda(file, dest_dir); - return dest_dir; + extract_conda(file, temp_extract_dir.path()); } else { throw std::runtime_error("Unknown package format (" + file.string() + ")"); } + fs::rename(temp_extract_dir.path(), dest_dir); + return dest_dir; } bool transmute(const fs::path& pkg_file, const fs::path& target, int compression_level)