From 3ecee680e4cbdf78649df9107da8c264f8902864 Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Fri, 27 Oct 2023 11:34:02 +0200 Subject: [PATCH 01/10] Reorder constructor member initialization --- src/cpp/lpnamer/problem_modifier/LinkProblemsGenerator.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cpp/lpnamer/problem_modifier/LinkProblemsGenerator.h b/src/cpp/lpnamer/problem_modifier/LinkProblemsGenerator.h index cb75ed2b0..431d6bf22 100644 --- a/src/cpp/lpnamer/problem_modifier/LinkProblemsGenerator.h +++ b/src/cpp/lpnamer/problem_modifier/LinkProblemsGenerator.h @@ -37,8 +37,8 @@ class LinkProblemsGenerator { _solver_name(std::move(solver_name)), lpDir_(lpDir), logger_(std::move(logger)), - solver_log_manager_(solver_log_manager), - rename_problems_(rename_problems) {} + rename_problems_(rename_problems), + solver_log_manager_(solver_log_manager) {} void treatloop(const std::filesystem::path& root, Couplings& couplings, const std::vector& mps_list, From 3a851e15475358e19fd1784e585bf3345a854dcb Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Fri, 27 Oct 2023 17:13:55 +0200 Subject: [PATCH 02/10] Encapsulate RunProblemGeneration in a class --- src/cpp/exe/full_run/main.cpp | 22 +---- src/cpp/exe/lpnamer/main.cpp | 35 +------- src/cpp/full_run/FullRunOptionsParser.cpp | 2 +- .../full_run/include/FullRunOptionsParser.h | 10 ++- src/cpp/lpnamer/main/CMakeLists.txt | 14 ++-- ...emGeneration.cpp => ProblemGeneration.cpp} | 58 +++++++++++--- .../main/ProblemGenerationExeOptions.cpp | 6 +- .../lpnamer/main/include/ProblemGeneration.h | 39 +++++++++ .../include/ProblemGenerationExeOptions.h | 6 +- .../main/include/ProblemGenerationOptions.h | 20 +++++ .../main/include/RunProblemGeneration.h | 27 ------- .../lpnamer/problem_modifier/CMakeLists.txt | 2 +- .../antares_xpansion/full_run_driver.py | 1 + .../ProblemGenerationExeOptionsTest.cpp | 80 +++++++++++++++---- tests/cpp/lp_namer/ProblemGenerationTest.cpp | 28 ++++++- 15 files changed, 228 insertions(+), 122 deletions(-) rename src/cpp/lpnamer/main/{RunProblemGeneration.cpp => ProblemGeneration.cpp} (80%) create mode 100644 src/cpp/lpnamer/main/include/ProblemGeneration.h create mode 100644 src/cpp/lpnamer/main/include/ProblemGenerationOptions.h delete mode 100644 src/cpp/lpnamer/main/include/RunProblemGeneration.h diff --git a/src/cpp/exe/full_run/main.cpp b/src/cpp/exe/full_run/main.cpp index fdc826b3e..997593efb 100644 --- a/src/cpp/exe/full_run/main.cpp +++ b/src/cpp/exe/full_run/main.cpp @@ -1,11 +1,10 @@ #include #include #include -#include #include "FullRunOptionsParser.h" +#include "ProblemGeneration.h" #include "ProblemGenerationLogger.h" -#include "RunProblemGeneration.h" #include "StudyUpdateRunner.h" #include "common_mpi.h" namespace po = boost::program_options; @@ -15,26 +14,11 @@ int main(int argc, char** argv) { mpi::communicator world; auto options_parser = FullRunOptionsParser(); std::filesystem::path xpansion_output_dir; - std::filesystem::path archive_path; options_parser.Parse(argc, argv); if (world.rank() == 0) { try { - xpansion_output_dir = options_parser.XpansionOutputDir(); - archive_path = options_parser.ArchivePath(); - - const auto log_file_path = - xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; - auto logger = ProblemGenerationLog::BuildLogger( - log_file_path, std::cout, "Full Run - Problem Generation"); - - auto master_formulation = options_parser.MasterFormulation(); - auto additionalConstraintFilename_l = - options_parser.AdditionalConstraintsFilename(); - auto weights_file = options_parser.WeightsFile(); - const auto unnamed_problems = options_parser.UnnamedProblems(); - RunProblemGeneration(xpansion_output_dir, master_formulation, - additionalConstraintFilename_l, archive_path, logger, - log_file_path, weights_file, unnamed_problems); + ProblemGeneration pbg(options_parser); + xpansion_output_dir = pbg.updateProblems(); } catch (std::exception& e) { std::cerr << "error: " << e.what() << std::endl; diff --git a/src/cpp/exe/lpnamer/main.cpp b/src/cpp/exe/lpnamer/main.cpp index dc940d881..4dd8fe136 100644 --- a/src/cpp/exe/lpnamer/main.cpp +++ b/src/cpp/exe/lpnamer/main.cpp @@ -1,45 +1,16 @@ #include +#include "ProblemGeneration.h" #include "ProblemGenerationExeOptions.h" -#include "ProblemGenerationLogger.h" -#include "RunProblemGeneration.h" - -static const std::string LP_DIRNAME = "lp"; - -void CreateDirectories(const std::filesystem::path& output_path) { - if (!std::filesystem::exists(output_path)) { - std::filesystem::create_directories(output_path); - } - auto lp_path = output_path / LP_DIRNAME; - if (!std::filesystem::exists(lp_path)) { - std::filesystem::create_directories(lp_path); - } -} int main(int argc, char** argv) { try { auto options_parser = ProblemGenerationExeOptions(); options_parser.Parse(argc, argv); - auto xpansion_output_dir = options_parser.XpansionOutputDir(); - auto archive_path = options_parser.ArchivePath(); - using namespace ProblemGenerationLog; - const auto log_file_path = - xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; - - CreateDirectories(xpansion_output_dir); - auto logger = ProblemGenerationLog::BuildLogger(log_file_path, std::cout, - "Problem Generation"); - - auto master_formulation = options_parser.MasterFormulation(); - auto additionalConstraintFilename_l = - options_parser.AdditionalConstraintsFilename(); - auto weights_file = options_parser.WeightsFile(); - auto unnamed_problems = options_parser.UnnamedProblems(); - RunProblemGeneration(xpansion_output_dir, master_formulation, - additionalConstraintFilename_l, archive_path, logger, - log_file_path, weights_file, unnamed_problems); + ProblemGeneration pbg(options_parser); + pbg.updateProblems(); return 0; } catch (std::exception& e) { diff --git a/src/cpp/full_run/FullRunOptionsParser.cpp b/src/cpp/full_run/FullRunOptionsParser.cpp index 8d1c3175f..9d79d669b 100644 --- a/src/cpp/full_run/FullRunOptionsParser.cpp +++ b/src/cpp/full_run/FullRunOptionsParser.cpp @@ -14,7 +14,7 @@ FullRunOptionsParser::FullRunOptionsParser() : ProblemGenerationExeOptions() { "path to json solution file"); } void FullRunOptionsParser::Parse(unsigned int argc, const char* const* argv) { - OptionsParser::Parse(argc, argv); + ProblemGenerationExeOptions::Parse(argc, argv); SetMethod(); } void FullRunOptionsParser::SetMethod() { diff --git a/src/cpp/full_run/include/FullRunOptionsParser.h b/src/cpp/full_run/include/FullRunOptionsParser.h index e5abdfe1d..c376c6e20 100644 --- a/src/cpp/full_run/include/FullRunOptionsParser.h +++ b/src/cpp/full_run/include/FullRunOptionsParser.h @@ -8,7 +8,7 @@ class FullRunOptionsParser : public ProblemGenerationExeOptions { public: FullRunOptionsParser(); - virtual ~FullRunOptionsParser() = default; + ~FullRunOptionsParser() override = default; void Parse(unsigned int argc, const char* const* argv) override; class FullRunOptionInvalidMethod : public std::runtime_error { public: @@ -18,11 +18,13 @@ class FullRunOptionsParser : public ProblemGenerationExeOptions { " is not supported! Please choose one of: " + FullRunOptionsParser::ListAvailalbleMethods()) {} }; - BENDERSMETHOD Method() const { return method_; }; - std::filesystem::path BendersOptionsFile() const { + [[nodiscard]] BENDERSMETHOD Method() const { return method_; }; + [[nodiscard]] std::filesystem::path BendersOptionsFile() const { return benders_options_file_; } - std::filesystem::path SolutionFile() const { return solutionFile_; } + [[nodiscard]] std::filesystem::path SolutionFile() const { + return solutionFile_; + } static inline std::string ListAvailalbleMethods() { return "benders,\n benders_by_batch,\n mergeMPS\n"; } diff --git a/src/cpp/lpnamer/main/CMakeLists.txt b/src/cpp/lpnamer/main/CMakeLists.txt index 6c01b7025..a3cfc55b5 100644 --- a/src/cpp/lpnamer/main/CMakeLists.txt +++ b/src/cpp/lpnamer/main/CMakeLists.txt @@ -11,8 +11,10 @@ # --------------------------------------------------------------------------- add_library (problem_generation_main STATIC - ${CMAKE_CURRENT_SOURCE_DIR}/RunProblemGeneration.cpp ${CMAKE_CURRENT_SOURCE_DIR}/ProblemGenerationExeOptions.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/ProblemGeneration.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/include/ProblemGeneration.h + ${CMAKE_CURRENT_SOURCE_DIR}/include/ProblemGenerationOptions.h ) target_link_libraries (problem_generation_main @@ -26,11 +28,11 @@ target_link_libraries (problem_generation_main get_target_property(helpers_include helpers INTERFACE_INCLUDE_DIRECTORIES) - target_include_directories (problem_generation_main - PUBLIC - ${CMAKE_CURRENT_SOURCE_DIR}/include - ${helpers_include} - ) +target_include_directories(problem_generation_main + PUBLIC + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${helpers_include} +) add_library (${PROJECT_NAME}::problem_generation_main ALIAS problem_generation_main) diff --git a/src/cpp/lpnamer/main/RunProblemGeneration.cpp b/src/cpp/lpnamer/main/ProblemGeneration.cpp similarity index 80% rename from src/cpp/lpnamer/main/RunProblemGeneration.cpp rename to src/cpp/lpnamer/main/ProblemGeneration.cpp index 527075c13..7c0e48f7b 100644 --- a/src/cpp/lpnamer/main/RunProblemGeneration.cpp +++ b/src/cpp/lpnamer/main/ProblemGeneration.cpp @@ -1,5 +1,8 @@ +// +// Created by marechaljas on 27/10/23. +// -#include "RunProblemGeneration.h" +#include "include/ProblemGeneration.h" #include #include @@ -15,6 +18,7 @@ #include "MasterGeneration.h" #include "MasterProblemBuilder.h" #include "MpsTxtWriter.h" +#include "ProblemGenerationLogger.h" #include "ProblemVariablesFromProblemAdapter.h" #include "ProblemVariablesZipAdapter.h" #include "StringManip.h" @@ -24,6 +28,41 @@ #include "ZipProblemsProviderAdapter.h" #include "config.h" +static const std::string LP_DIRNAME = "lp"; + +void CreateDirectories(const std::filesystem::path& output_path) { + if (!std::filesystem::exists(output_path)) { + std::filesystem::create_directories(output_path); + } + auto lp_path = output_path / LP_DIRNAME; + if (!std::filesystem::exists(lp_path)) { + std::filesystem::create_directories(lp_path); + } +} + +ProblemGeneration::ProblemGeneration(ProblemGenerationOptions& options) + : options_(options) {} +std::filesystem::path ProblemGeneration::updateProblems() { + auto xpansion_output_dir = options_.XpansionOutputDir(); + auto archive_path = options_.ArchivePath(); + const auto log_file_path = + xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; + + CreateDirectories(xpansion_output_dir); + auto logger = ProblemGenerationLog::BuildLogger(log_file_path, std::cout, + "Problem Generation"); + + auto master_formulation = options_.MasterFormulation(); + auto additionalConstraintFilename_l = + options_.AdditionalConstraintsFilename(); + auto weights_file = options_.WeightsFile(); + auto unnamed_problems = options_.UnnamedProblems(); + RunProblemGeneration(xpansion_output_dir, master_formulation, + additionalConstraintFilename_l, archive_path, logger, + log_file_path, weights_file, unnamed_problems); + return xpansion_output_dir; +} + struct Version { explicit Version(std::string_view version) { auto split_version = StringManip::split(StringManip::trim(version), '.'); @@ -58,7 +97,7 @@ struct Version { std::shared_ptr InstantiateZipReader( const std::filesystem::path& antares_archive_path); -void ProcessWeights( +void ProblemGeneration::ProcessWeights( const std::filesystem::path& xpansion_output_dir, const std::filesystem::path& antares_archive_path, const std::filesystem::path& weights_file, @@ -77,7 +116,7 @@ void ProcessWeights( yearly_weight_writer.CreateWeightFile(); } -void ExtractUtilsFiles( +void ProblemGeneration::ExtractUtilsFiles( const std::filesystem::path& antares_archive_path, const std::filesystem::path& xpansion_output_dir, ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger) { @@ -126,7 +165,7 @@ std::vector> getXpansionProblems( return adapter->provideProblems(solver_name, solver_log_manager); } -void RunProblemGeneration( +void ProblemGeneration::RunProblemGeneration( const std::filesystem::path& xpansion_output_dir, const std::string& master_formulation, const std::string& additionalConstraintFilename_l, @@ -171,13 +210,12 @@ void RunProblemGeneration( Couplings couplings; LinkProblemsGenerator linkProblemsGenerator( lpDir_, links, solver_name, logger, solver_log_manager, rename_problems); - std::shared_ptr reader = - InstantiateZipReader(antares_archive_path); + std::shared_ptr reader = + InstantiateZipReader(antares_archive_path); - /* Main stuff */ - std::vector> xpansion_problems = - getXpansionProblems(solver_log_manager, solver_name, mpsList, lpDir_, - reader); + /* Main stuff */ + std::vector> xpansion_problems = getXpansionProblems( + solver_log_manager, solver_name, mpsList, lpDir_, reader); std::vector, ProblemData>> problems_and_data; diff --git a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp index 71efbef91..4d2859844 100644 --- a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp +++ b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp @@ -20,4 +20,8 @@ ProblemGenerationExeOptions::ProblemGenerationExeOptions() "user weights file")("unnamed-problems,n", po::bool_switch(&unnamed_problems_), "use this option if unnamed problems are provided"); -} \ No newline at end of file +} +void ProblemGenerationExeOptions::Parse(unsigned int argc, + const char *const *argv) { + OptionsParser::Parse(argc, argv); +} diff --git a/src/cpp/lpnamer/main/include/ProblemGeneration.h b/src/cpp/lpnamer/main/include/ProblemGeneration.h new file mode 100644 index 000000000..f9ac5618f --- /dev/null +++ b/src/cpp/lpnamer/main/include/ProblemGeneration.h @@ -0,0 +1,39 @@ +// +// Created by marechaljas on 27/10/23. +// + +#pragma once + +#include +#include + +#include "ProblemGenerationExeOptions.h" +#include "ProblemGenerationLogger.h" +#include "ProblemGenerationOptions.h" + +class ProblemGeneration { + public: + explicit ProblemGeneration(ProblemGenerationOptions& options); + std::filesystem::path updateProblems(); + ProblemGenerationOptions& options_; + + virtual void RunProblemGeneration( + const std::filesystem::path& xpansion_output_dir, + const std::string& master_formulation, + const std::string& additionalConstraintFilename_l, + const std::filesystem::path& archive_path, + ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger, + const std::filesystem::path& log_file_path, + const std::filesystem::path& weights_file, bool unnamed_problems); + + private: + void ProcessWeights( + const std::filesystem::path& xpansion_output_dir, + const std::filesystem::path& antares_archive_path, + const std::filesystem::path& weights_file, + ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger); + void ExtractUtilsFiles( + const std::filesystem::path& antares_archive_path, + const std::filesystem::path& xpansion_output_dir, + ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger); +}; diff --git a/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h b/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h index 56345ae36..a1f0c6711 100644 --- a/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h +++ b/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h @@ -4,8 +4,10 @@ #include #include "OptionsParser.h" +#include "ProblemGenerationOptions.h" -class ProblemGenerationExeOptions : public OptionsParser { +class ProblemGenerationExeOptions : public OptionsParser, + public ProblemGenerationOptions { private: std::filesystem::path xpansion_output_dir_; std::string master_formulation_; @@ -30,5 +32,7 @@ class ProblemGenerationExeOptions : public OptionsParser { std::filesystem::path WeightsFile() const { return weights_file_; } std::vector ActiveYears() const { return active_years_; } bool UnnamedProblems() const { return unnamed_problems_; } + + void Parse(unsigned int argc, const char *const *argv) override; }; #endif // ANTARES_XPANSION_SRC_CPP_LPNAMER_MAIN_INCLUDE_PROBLEMGENERATIONEXEOPTIONS_H diff --git a/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h b/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h new file mode 100644 index 000000000..aa32d223a --- /dev/null +++ b/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h @@ -0,0 +1,20 @@ +// +// Created by marechaljas on 27/10/23. +// + +#pragma once + +#include +#include + +class ProblemGenerationOptions { + public: + virtual ~ProblemGenerationOptions() = default; + [[nodiscard]] virtual std::filesystem::path XpansionOutputDir() const = 0; + [[nodiscard]] virtual std::string MasterFormulation() const = 0; + [[nodiscard]] virtual std::string AdditionalConstraintsFilename() const = 0; + [[nodiscard]] virtual std::filesystem::path ArchivePath() const = 0; + [[nodiscard]] virtual std::filesystem::path WeightsFile() const = 0; + [[nodiscard]] virtual std::vector ActiveYears() const = 0; + [[nodiscard]] virtual bool UnnamedProblems() const = 0; +}; diff --git a/src/cpp/lpnamer/main/include/RunProblemGeneration.h b/src/cpp/lpnamer/main/include/RunProblemGeneration.h deleted file mode 100644 index d4eac444f..000000000 --- a/src/cpp/lpnamer/main/include/RunProblemGeneration.h +++ /dev/null @@ -1,27 +0,0 @@ -#ifndef ANTARES_XPANSION_SRC_CPP_LPNAMER_MAIN_INCLUDE_RUNPROBLEMGENERATION_H -#define ANTARES_XPANSION_SRC_CPP_LPNAMER_MAIN_INCLUDE_RUNPROBLEMGENERATION_H -#include -#include - -#include "ProblemGenerationExeOptions.h" -#include "ProblemGenerationLogger.h" - -void RunProblemGeneration( - const std::filesystem::path& xpansion_output_dir, - const std::string& master_formulation, - const std::string& additionalConstraintFilename_l, - const std::filesystem::path& archive_path, - ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger, - const std::filesystem::path& log_file_path, - const std::filesystem::path& weights_file, bool unnamed_problems); -void ProcessWeights( - const std::filesystem::path& xpansion_output_dir, - const std::filesystem::path& antares_archive_path, - const std::filesystem::path& weights_file, - ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger); -void ExtractUtilsFiles( - const std::filesystem::path& antares_archive_path, - const std::filesystem::path& xpansion_output_dir, - ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger); - -#endif // ANTARES_XPANSION_SRC_CPP_LPNAMER_MAIN_INCLUDE_RUNPROBLEMGENERATION_H diff --git a/src/cpp/lpnamer/problem_modifier/CMakeLists.txt b/src/cpp/lpnamer/problem_modifier/CMakeLists.txt index 291188a21..bb5d936e2 100644 --- a/src/cpp/lpnamer/problem_modifier/CMakeLists.txt +++ b/src/cpp/lpnamer/problem_modifier/CMakeLists.txt @@ -52,7 +52,7 @@ add_library(lp_namer_problem_modifier STATIC ${CMAKE_CURRENT_SOURCE_DIR}/ZipProblemsProviderAdapter.h ${CMAKE_CURRENT_SOURCE_DIR}/ProblemVariablesFromProblemAdapter.cpp ${CMAKE_CURRENT_SOURCE_DIR}/ProblemVariablesFromProblemAdapter.h - ) +) target_include_directories (lp_namer_problem_modifier PUBLIC diff --git a/src/python/antares_xpansion/full_run_driver.py b/src/python/antares_xpansion/full_run_driver.py index d88baa2fc..d2fcf88aa 100644 --- a/src/python/antares_xpansion/full_run_driver.py +++ b/src/python/antares_xpansion/full_run_driver.py @@ -66,6 +66,7 @@ def run(self): os.chdir(lp_path) self.logger.info(f"Current directory is now: {os.getcwd()}") + self.logger.info(f"Command is {self.full_command()}") ret = subprocess.run( self.full_command(), shell=False, stdout=sys.stdout, stderr=sys.stderr, encoding='utf-8') diff --git a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp index 4b8316237..964fa98e8 100644 --- a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp @@ -1,3 +1,4 @@ +#include "ProblemGeneration.h" #include "ProblemGenerationExeOptions.h" #include "gtest/gtest.h" @@ -8,7 +9,42 @@ class ProblemGenerationExeOptionsTest : public ::testing::Test { ProblemGenerationExeOptions problem_generation_options_parser_; }; -TEST_F(ProblemGenerationExeOptionsTest, WithOutArchiveOption) { +class ProblemGenerationSpyAndMock : public ProblemGeneration { + public: + ProblemGenerationSpyAndMock(ProblemGenerationExeOptions& options) + : ProblemGeneration(options) {} + void RunProblemGeneration( + const std::filesystem::path& xpansion_output_dir, + const std::string& master_formulation, + const std::string& additionalConstraintFilename_l, + const std::filesystem::path& archive_path, + ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger, + const std::filesystem::path& log_file_path, + const std::filesystem::path& weights_file, + bool unnamed_problems) override { + xpansion_output_dir_ = xpansion_output_dir; + master_formulation_ = master_formulation; + additionalConstraintFilename_l_ = additionalConstraintFilename_l; + archive_path_ = archive_path; + log_file_path_ = log_file_path; + weights_file_ = weights_file; + unnamed_problems_ = unnamed_problems; + + // ProblemGeneration::RunProblemGeneration( + // xpansion_output_dir, master_formulation, + // additionalConstraintFilename_l, archive_path, logger, + // log_file_path, weights_file, unnamed_problems); + } + std::filesystem::path xpansion_output_dir_; + std::string master_formulation_; + std::string additionalConstraintFilename_l_; + std::filesystem::path archive_path_; + std::filesystem::path log_file_path_; + std::filesystem::path weights_file_; + bool unnamed_problems_; +}; + +TEST_F(ProblemGenerationExeOptionsTest, WithoutArchiveOption) { char argv = 'c'; char* pargv = &argv; char** ppargv = &pargv; @@ -19,7 +55,7 @@ TEST_F(ProblemGenerationExeOptionsTest, WithOutArchiveOption) { std::string("the option '--archive' is required but missing")); } } -TEST_F(ProblemGenerationExeOptionsTest, WithOutOutputOption) { +TEST_F(ProblemGenerationExeOptionsTest, WithoutOutputOption) { const char argv0[] = "lp_namer.exe"; const char argv1[] = "--archive"; const char argv2[] = "something"; @@ -43,18 +79,28 @@ TEST_F(ProblemGenerationExeOptionsTest, MasterFormulationDefaultValue) { ASSERT_EQ(problem_generation_options_parser_.MasterFormulation(), std::string("relaxed")); } -// TEST_F(ProblemGenerationExeOptionsTest, -// AdditionalConstraintsFilenameIsNotEmptyIfGiven) { -// const char argv0[] = "lp.exe "; -// const char argv1[] = "--output"; -// const char argv2[] = "something"; -// const char argv3[] = "-e"; -// std::vector ppargv = {argv0, argv1, argv2, argv3}; -// try { -// problem_generation_options_parser_.Parse(4, ppargv.data()); -// } catch (const std::exception& e) { -// EXPECT_EQ(e.what(), -// std::string("the required argument for option -// '--exclusion-files' is missing")); -// } -// } + +TEST_F(ProblemGenerationExeOptionsTest, + OutputAndArchiveParameters_useProperValues) { + auto test_root = + std::filesystem::temp_directory_path() / std::tmpnam(nullptr); + auto archive = std::string(tmpnam(nullptr)) + "study.zip"; + auto output_path = test_root / "study-Xpansion"; + + const char argv0[] = "lp.exe "; + const char argv1[] = "--archive"; + auto argv2 = archive; + const char argv3[] = "--output"; + auto argv4 = output_path.string(); + std::vector ppargv = {argv0, argv1, argv2.c_str(), argv3, + argv4.c_str()}; + problem_generation_options_parser_.Parse(5, ppargv.data()); + + ProblemGenerationSpyAndMock pbg(problem_generation_options_parser_); + pbg.updateProblems(); + + EXPECT_EQ(pbg.archive_path_, archive); + EXPECT_EQ(pbg.xpansion_output_dir_, output_path); + EXPECT_TRUE(std::filesystem::exists(output_path)); + EXPECT_TRUE(std::filesystem::exists(output_path / "lp")); +} diff --git a/tests/cpp/lp_namer/ProblemGenerationTest.cpp b/tests/cpp/lp_namer/ProblemGenerationTest.cpp index 2e8506965..6cd0afce6 100644 --- a/tests/cpp/lp_namer/ProblemGenerationTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationTest.cpp @@ -6,9 +6,28 @@ #include "ArchiveIO.h" #include "LoggerBuilder.h" -#include "RunProblemGeneration.h" +#include "ProblemGeneration.h" #include "gtest/gtest.h" +class InMemoryOption : public ProblemGenerationOptions { + public: + std::filesystem::path XpansionOutputDir() const override { + return std::filesystem::path(); + } + std::string MasterFormulation() const override { return std::string(); } + std::string AdditionalConstraintsFilename() const override { + return std::string(); + } + std::filesystem::path ArchivePath() const override { + return std::filesystem::path(); + } + std::filesystem::path WeightsFile() const override { + return std::filesystem::path(); + } + std::vector ActiveYears() const override { return std::vector(); } + bool UnnamedProblems() const override { return false; } +}; + TEST(InitializationTest, FoldersAreEmpty) { auto workingDirectory = std::filesystem::temp_directory_path() / std::tmpnam(nullptr); @@ -22,10 +41,13 @@ TEST(InitializationTest, FoldersAreEmpty) { std::filesystem::create_directories(xpansionDirectory); std::ofstream emptyfile(xpansionDirectory / "garbage.txt"); + InMemoryOption options; + ProblemGeneration pbg(options); + EXPECT_TRUE(std::filesystem::exists(xpansionDirectory / "garbage.txt")); EXPECT_THROW( - RunProblemGeneration(simulationDirectory, "integer", "", "", logger, - simulationDirectory / "logs.txt", "", false), + pbg.RunProblemGeneration(simulationDirectory, "integer", "", "", logger, + simulationDirectory / "logs.txt", "", false), ArchiveIOGeneralException); EXPECT_FALSE(std::filesystem::exists(simulationDirectory / "garbage.txt")); From eaa558eea51163410db7cd078d2de7ca937e22fb Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Thu, 2 Nov 2023 17:20:09 +0100 Subject: [PATCH 03/10] Deduce xpansion-output dir from archive path --- src/cpp/lpnamer/main/ProblemGeneration.cpp | 8 ++++++ .../main/ProblemGenerationExeOptions.cpp | 3 +- .../ProblemGenerationExeOptionsTest.cpp | 28 +++++++++++++++---- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/cpp/lpnamer/main/ProblemGeneration.cpp b/src/cpp/lpnamer/main/ProblemGeneration.cpp index 7c0e48f7b..ea1417e4f 100644 --- a/src/cpp/lpnamer/main/ProblemGeneration.cpp +++ b/src/cpp/lpnamer/main/ProblemGeneration.cpp @@ -45,6 +45,14 @@ ProblemGeneration::ProblemGeneration(ProblemGenerationOptions& options) std::filesystem::path ProblemGeneration::updateProblems() { auto xpansion_output_dir = options_.XpansionOutputDir(); auto archive_path = options_.ArchivePath(); + + if (xpansion_output_dir.string().empty() && !archive_path.string().empty()) { + xpansion_output_dir = archive_path; + xpansion_output_dir = xpansion_output_dir.replace_filename( + xpansion_output_dir.stem().replace_extension("").string() + + "-Xpansion"); + } + const auto log_file_path = xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; diff --git a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp index 4d2859844..49b8b481d 100644 --- a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp +++ b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp @@ -4,8 +4,7 @@ namespace po = boost::program_options; ProblemGenerationExeOptions::ProblemGenerationExeOptions() : OptionsParser("Problem Generation exe") { AddOptions()("help,h", "produce help message")( - "output,o", - po::value(&xpansion_output_dir_)->required(), + "output,o", po::value(&xpansion_output_dir_), "antares-xpansion study output")( "archive,a", po::value(&archive_path_)->required(), "antares-xpansion study zip")( diff --git a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp index 964fa98e8..8dfec87d6 100644 --- a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp @@ -29,11 +29,6 @@ class ProblemGenerationSpyAndMock : public ProblemGeneration { log_file_path_ = log_file_path; weights_file_ = weights_file; unnamed_problems_ = unnamed_problems; - - // ProblemGeneration::RunProblemGeneration( - // xpansion_output_dir, master_formulation, - // additionalConstraintFilename_l, archive_path, logger, - // log_file_path, weights_file, unnamed_problems); } std::filesystem::path xpansion_output_dir_; std::string master_formulation_; @@ -104,3 +99,26 @@ TEST_F(ProblemGenerationExeOptionsTest, EXPECT_TRUE(std::filesystem::exists(output_path)); EXPECT_TRUE(std::filesystem::exists(output_path / "lp")); } + +TEST_F(ProblemGenerationExeOptionsTest, + OutputAndArchiveParameters_deduceOuputFromArchive) { + auto test_root = + std::filesystem::temp_directory_path() / std::tmpnam(nullptr); + auto archive = test_root / "study.zip"; + auto output_path = test_root / "study-Xpansion"; + + const char argv0[] = "lp.exe "; + const char argv1[] = "--archive"; + auto argv2 = archive; + + std::vector ppargv = {argv0, argv1, argv2.c_str()}; + problem_generation_options_parser_.Parse(3, ppargv.data()); + + ProblemGenerationSpyAndMock pbg(problem_generation_options_parser_); + pbg.updateProblems(); + + EXPECT_EQ(pbg.archive_path_, archive); + EXPECT_EQ(pbg.xpansion_output_dir_, output_path); + EXPECT_TRUE(std::filesystem::exists(output_path)); + EXPECT_TRUE(std::filesystem::exists(output_path / "lp")); +} From 4b43d4cb9edfb965420789c64f96fe32eedf91ee Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Thu, 2 Nov 2023 17:31:33 +0100 Subject: [PATCH 04/10] Refactor --- src/cpp/lpnamer/main/ProblemGeneration.cpp | 19 +++++++++++++------ .../lpnamer/main/include/ProblemGeneration.h | 3 +++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/cpp/lpnamer/main/ProblemGeneration.cpp b/src/cpp/lpnamer/main/ProblemGeneration.cpp index ea1417e4f..0bb7566d8 100644 --- a/src/cpp/lpnamer/main/ProblemGeneration.cpp +++ b/src/cpp/lpnamer/main/ProblemGeneration.cpp @@ -46,12 +46,8 @@ std::filesystem::path ProblemGeneration::updateProblems() { auto xpansion_output_dir = options_.XpansionOutputDir(); auto archive_path = options_.ArchivePath(); - if (xpansion_output_dir.string().empty() && !archive_path.string().empty()) { - xpansion_output_dir = archive_path; - xpansion_output_dir = xpansion_output_dir.replace_filename( - xpansion_output_dir.stem().replace_extension("").string() + - "-Xpansion"); - } + xpansion_output_dir = + deduceXpansionDirIfEmpty(xpansion_output_dir, archive_path); const auto log_file_path = xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; @@ -70,6 +66,17 @@ std::filesystem::path ProblemGeneration::updateProblems() { log_file_path, weights_file, unnamed_problems); return xpansion_output_dir; } +std::filesystem::path ProblemGeneration::deduceXpansionDirIfEmpty( + std::filesystem::path xpansion_output_dir, + const std::filesystem::path& archive_path) { + if (xpansion_output_dir.string().empty() && !archive_path.string().empty()) { + auto deduced_dir = archive_path; + deduced_dir = deduced_dir.replace_filename( + deduced_dir.stem().replace_extension("").string() + "-Xpansion"); + return deduced_dir; + } + return xpansion_output_dir; +} struct Version { explicit Version(std::string_view version) { diff --git a/src/cpp/lpnamer/main/include/ProblemGeneration.h b/src/cpp/lpnamer/main/include/ProblemGeneration.h index f9ac5618f..8888e75dc 100644 --- a/src/cpp/lpnamer/main/include/ProblemGeneration.h +++ b/src/cpp/lpnamer/main/include/ProblemGeneration.h @@ -36,4 +36,7 @@ class ProblemGeneration { const std::filesystem::path& antares_archive_path, const std::filesystem::path& xpansion_output_dir, ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger); + static std::filesystem::path deduceXpansionDirIfEmpty( + std::filesystem::path xpansion_output_dir, + const std::filesystem::path& archive_path); }; From d96ba05f87f21838c5c94e05020d8d4b3d7f68a6 Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Thu, 2 Nov 2023 17:54:21 +0100 Subject: [PATCH 05/10] Deduce archive name from xpansion output name --- src/cpp/lpnamer/main/ProblemGeneration.cpp | 9 +++++++- .../main/ProblemGenerationExeOptions.cpp | 2 +- tests/cpp/full_run/FullRunTest.cpp | 10 -------- .../ProblemGenerationExeOptionsTest.cpp | 23 +++++++++++++++++++ 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/cpp/lpnamer/main/ProblemGeneration.cpp b/src/cpp/lpnamer/main/ProblemGeneration.cpp index 0bb7566d8..c4d94e39a 100644 --- a/src/cpp/lpnamer/main/ProblemGeneration.cpp +++ b/src/cpp/lpnamer/main/ProblemGeneration.cpp @@ -48,6 +48,13 @@ std::filesystem::path ProblemGeneration::updateProblems() { xpansion_output_dir = deduceXpansionDirIfEmpty(xpansion_output_dir, archive_path); + if (archive_path.empty() && !xpansion_output_dir.empty()) { + archive_path = xpansion_output_dir; + auto dir_name = archive_path.stem().string(); + dir_name = dir_name.substr(0, dir_name.find("-Xpansion")); + archive_path = + archive_path.replace_filename(dir_name).replace_extension(".zip"); + } const auto log_file_path = xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; @@ -69,7 +76,7 @@ std::filesystem::path ProblemGeneration::updateProblems() { std::filesystem::path ProblemGeneration::deduceXpansionDirIfEmpty( std::filesystem::path xpansion_output_dir, const std::filesystem::path& archive_path) { - if (xpansion_output_dir.string().empty() && !archive_path.string().empty()) { + if (xpansion_output_dir.empty() && !archive_path.empty()) { auto deduced_dir = archive_path; deduced_dir = deduced_dir.replace_filename( deduced_dir.stem().replace_extension("").string() + "-Xpansion"); diff --git a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp index 49b8b481d..425a30b5b 100644 --- a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp +++ b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp @@ -6,7 +6,7 @@ ProblemGenerationExeOptions::ProblemGenerationExeOptions() AddOptions()("help,h", "produce help message")( "output,o", po::value(&xpansion_output_dir_), "antares-xpansion study output")( - "archive,a", po::value(&archive_path_)->required(), + "archive,a", po::value(&archive_path_), "antares-xpansion study zip")( "formulation,f", po::value(&master_formulation_)->default_value("relaxed"), diff --git a/tests/cpp/full_run/FullRunTest.cpp b/tests/cpp/full_run/FullRunTest.cpp index 5570abded..6ef4ff737 100644 --- a/tests/cpp/full_run/FullRunTest.cpp +++ b/tests/cpp/full_run/FullRunTest.cpp @@ -32,16 +32,6 @@ class FullRunOptionsParserTestParameterizedMethod FullRunOptionsParser full_run_options_options_parser_; }; -TEST_F(FullRunOptionsParserTest, ThatArchiveFileIsRequired) { - const char argv0[] = "full_run.exe"; - std::vector ppargv = {argv0}; - try { - full_run_options_options_parser_.Parse(1, ppargv.data()); - } catch (const std::exception& e) { - EXPECT_EQ(e.what(), - std::string("the option '--archive' is required but missing")); - } -} TEST_F(FullRunOptionsParserTest, ThatBendersOptionFileIsRequired) { const char argv0[] = "full_run.exe"; const char argv1[] = "--archive"; diff --git a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp index 8dfec87d6..c5d70efe7 100644 --- a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp @@ -122,3 +122,26 @@ TEST_F(ProblemGenerationExeOptionsTest, EXPECT_TRUE(std::filesystem::exists(output_path)); EXPECT_TRUE(std::filesystem::exists(output_path / "lp")); } + +TEST_F(ProblemGenerationExeOptionsTest, + OutputAndArchiveParameters_deduceArchiveFromOutputDir) { + auto test_root = + std::filesystem::temp_directory_path() / std::tmpnam(nullptr); + auto archive = test_root / "study.zip"; + auto output_path = test_root / "study-Xpansion"; + + const char argv0[] = "lp.exe "; + const char argv1[] = "--output"; + auto argv2 = output_path.string(); + + std::vector ppargv = {argv0, argv1, argv2.c_str()}; + problem_generation_options_parser_.Parse(3, ppargv.data()); + + ProblemGenerationSpyAndMock pbg(problem_generation_options_parser_); + pbg.updateProblems(); + + EXPECT_EQ(pbg.archive_path_, archive); + EXPECT_EQ(pbg.xpansion_output_dir_, output_path); + EXPECT_TRUE(std::filesystem::exists(output_path)); + EXPECT_TRUE(std::filesystem::exists(output_path / "lp")); +} \ No newline at end of file From d92f948a319018175704163f9eb020e7dc4ad0a3 Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Fri, 3 Nov 2023 15:09:24 +0100 Subject: [PATCH 06/10] Handle not deductible archive file name --- src/cpp/lpnamer/main/ProblemGeneration.cpp | 56 ++++++++++++++----- .../lpnamer/main/include/ProblemGeneration.h | 12 ++++ .../ProblemGenerationExeOptionsTest.cpp | 42 ++++++++++++++ 3 files changed, 95 insertions(+), 15 deletions(-) diff --git a/src/cpp/lpnamer/main/ProblemGeneration.cpp b/src/cpp/lpnamer/main/ProblemGeneration.cpp index c4d94e39a..5ab3109ed 100644 --- a/src/cpp/lpnamer/main/ProblemGeneration.cpp +++ b/src/cpp/lpnamer/main/ProblemGeneration.cpp @@ -43,23 +43,16 @@ void CreateDirectories(const std::filesystem::path& output_path) { ProblemGeneration::ProblemGeneration(ProblemGenerationOptions& options) : options_(options) {} std::filesystem::path ProblemGeneration::updateProblems() { - auto xpansion_output_dir = options_.XpansionOutputDir(); - auto archive_path = options_.ArchivePath(); + const auto xpansion_output_dir = options_.XpansionOutputDir(); + const auto archive_path = options_.ArchivePath(); - xpansion_output_dir = + auto deduced_xpansion_output_dir = deduceXpansionDirIfEmpty(xpansion_output_dir, archive_path); - if (archive_path.empty() && !xpansion_output_dir.empty()) { - archive_path = xpansion_output_dir; - auto dir_name = archive_path.stem().string(); - dir_name = dir_name.substr(0, dir_name.find("-Xpansion")); - archive_path = - archive_path.replace_filename(dir_name).replace_extension(".zip"); - } const auto log_file_path = xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; - CreateDirectories(xpansion_output_dir); + CreateDirectories(deduced_xpansion_output_dir); auto logger = ProblemGenerationLog::BuildLogger(log_file_path, std::cout, "Problem Generation"); @@ -68,10 +61,40 @@ std::filesystem::path ProblemGeneration::updateProblems() { options_.AdditionalConstraintsFilename(); auto weights_file = options_.WeightsFile(); auto unnamed_problems = options_.UnnamedProblems(); - RunProblemGeneration(xpansion_output_dir, master_formulation, - additionalConstraintFilename_l, archive_path, logger, - log_file_path, weights_file, unnamed_problems); - return xpansion_output_dir; + std::filesystem::path deduced_archive_path; + try { + deduced_archive_path = + deduceArchivePathIfEmpty(deduced_xpansion_output_dir, archive_path); + } catch (const MismatchedParameters& m) { + (*logger)(LogUtils::LOGLEVEL::ERR) << m.what(); + throw; + } + + RunProblemGeneration(deduced_xpansion_output_dir, master_formulation, + additionalConstraintFilename_l, deduced_archive_path, + logger, log_file_path, weights_file, unnamed_problems); + return deduced_xpansion_output_dir; +} +std::filesystem::path ProblemGeneration::deduceArchivePathIfEmpty( + const std::filesystem::path& xpansion_output_dir, + const std::filesystem::path& archive_path) { + if (archive_path.empty() && !xpansion_output_dir.empty()) { + if (xpansion_output_dir.string().find("-Xpansion") == std::string::npos) { + auto log_location = LOGLOCATION; + auto msg = + "Archive path is missing and output path does not contains" + " \"-Xpansion\" suffixe. Can't deduce archive file name."; + throw MismatchedParameters(msg, log_location); + } + auto deduced_archive_path = xpansion_output_dir; + auto dir_name = deduced_archive_path.stem().string(); + dir_name = dir_name.substr(0, dir_name.find("-Xpansion")); + deduced_archive_path = + deduced_archive_path.replace_filename(dir_name).replace_extension( + ".zip"); + return deduced_archive_path; + } + return archive_path; } std::filesystem::path ProblemGeneration::deduceXpansionDirIfEmpty( std::filesystem::path xpansion_output_dir, @@ -280,3 +303,6 @@ std::shared_ptr InstantiateZipReader( reader->Open(); return reader; } +ProblemGeneration::MismatchedParameters::MismatchedParameters( + const std::string& err_message, const std::string& log_location) + : XpansionError(err_message, log_location) {} diff --git a/src/cpp/lpnamer/main/include/ProblemGeneration.h b/src/cpp/lpnamer/main/include/ProblemGeneration.h index 8888e75dc..867a96dbd 100644 --- a/src/cpp/lpnamer/main/include/ProblemGeneration.h +++ b/src/cpp/lpnamer/main/include/ProblemGeneration.h @@ -26,6 +26,15 @@ class ProblemGeneration { const std::filesystem::path& log_file_path, const std::filesystem::path& weights_file, bool unnamed_problems); + class MismatchedParameters + : public LogUtils::XpansionError { + using LogUtils::XpansionError::XpansionError; + + public: + explicit MismatchedParameters(const std::string& err_message, + const std::string& log_location); + }; + private: void ProcessWeights( const std::filesystem::path& xpansion_output_dir, @@ -39,4 +48,7 @@ class ProblemGeneration { static std::filesystem::path deduceXpansionDirIfEmpty( std::filesystem::path xpansion_output_dir, const std::filesystem::path& archive_path); + static std::filesystem::path deduceArchivePathIfEmpty( + const std::filesystem::path& xpansion_output_dir, + const std::filesystem::path& archive_path); }; diff --git a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp index c5d70efe7..89bc2fd5b 100644 --- a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp @@ -144,4 +144,46 @@ TEST_F(ProblemGenerationExeOptionsTest, EXPECT_EQ(pbg.xpansion_output_dir_, output_path); EXPECT_TRUE(std::filesystem::exists(output_path)); EXPECT_TRUE(std::filesystem::exists(output_path / "lp")); +} + +bool Constains(std::ifstream& file, const std::string& string) { + std::string line; + + while (std::getline(file, line)) { + if (line.find(string) != std::string::npos) { + return true; + } + } + return false; +} + +TEST_F( + ProblemGenerationExeOptionsTest, + OutputAndArchiveParameters_cantDeduceArchiveFromOutputDirWithoutRegularSuffixe) { + auto test_root = + std::filesystem::temp_directory_path() / std::tmpnam(nullptr); + auto archive = test_root / "study.zip"; + auto output_path = test_root / "study-out"; + + const char argv0[] = "lp.exe "; + const char argv1[] = "--output"; + auto argv2 = output_path.string(); + + std::vector ppargv = {argv0, argv1, argv2.c_str()}; + problem_generation_options_parser_.Parse(3, ppargv.data()); + + ProblemGenerationSpyAndMock pbg(problem_generation_options_parser_); + + EXPECT_THROW(pbg.updateProblems(), ProblemGeneration::MismatchedParameters); + + EXPECT_TRUE(pbg.archive_path_.empty()); // Can't deduce + EXPECT_TRUE(pbg.xpansion_output_dir_ + .empty()); // Exception occurres before RunProblemGeneration + // We expect to have created directories to log properly + EXPECT_TRUE(std::filesystem::exists(output_path)); + EXPECT_TRUE(std::filesystem::exists(output_path / "lp")); + + std::ifstream logfile(output_path / "lp" / "ProblemGenerationLog.txt"); + EXPECT_TRUE(Constains( + logfile, "Archive path is missing and output path does not contains")); } \ No newline at end of file From 79f6f4360c1986c863e9b1545cd1796233236bca Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Fri, 3 Nov 2023 15:46:24 +0100 Subject: [PATCH 07/10] Move output and archive deduction in ProblemGenerationExeOptions --- src/cpp/lpnamer/main/ProblemGeneration.cpp | 43 ++----------------- .../main/ProblemGenerationExeOptions.cpp | 38 ++++++++++++++++ .../lpnamer/main/include/ProblemGeneration.h | 15 ------- .../include/ProblemGenerationExeOptions.h | 7 +++ .../main/include/ProblemGenerationOptions.h | 15 +++++++ .../ProblemGenerationExeOptionsTest.cpp | 3 +- tests/cpp/lp_namer/ProblemGenerationTest.cpp | 10 +++++ 7 files changed, 76 insertions(+), 55 deletions(-) diff --git a/src/cpp/lpnamer/main/ProblemGeneration.cpp b/src/cpp/lpnamer/main/ProblemGeneration.cpp index 5ab3109ed..539edad2b 100644 --- a/src/cpp/lpnamer/main/ProblemGeneration.cpp +++ b/src/cpp/lpnamer/main/ProblemGeneration.cpp @@ -47,7 +47,7 @@ std::filesystem::path ProblemGeneration::updateProblems() { const auto archive_path = options_.ArchivePath(); auto deduced_xpansion_output_dir = - deduceXpansionDirIfEmpty(xpansion_output_dir, archive_path); + options_.deduceXpansionDirIfEmpty(xpansion_output_dir, archive_path); const auto log_file_path = xpansion_output_dir / "lp" / "ProblemGenerationLog.txt"; @@ -63,9 +63,9 @@ std::filesystem::path ProblemGeneration::updateProblems() { auto unnamed_problems = options_.UnnamedProblems(); std::filesystem::path deduced_archive_path; try { - deduced_archive_path = - deduceArchivePathIfEmpty(deduced_xpansion_output_dir, archive_path); - } catch (const MismatchedParameters& m) { + deduced_archive_path = options_.deduceArchivePathIfEmpty( + deduced_xpansion_output_dir, archive_path); + } catch (const ProblemGenerationOptions::MismatchedParameters& m) { (*logger)(LogUtils::LOGLEVEL::ERR) << m.what(); throw; } @@ -75,38 +75,6 @@ std::filesystem::path ProblemGeneration::updateProblems() { logger, log_file_path, weights_file, unnamed_problems); return deduced_xpansion_output_dir; } -std::filesystem::path ProblemGeneration::deduceArchivePathIfEmpty( - const std::filesystem::path& xpansion_output_dir, - const std::filesystem::path& archive_path) { - if (archive_path.empty() && !xpansion_output_dir.empty()) { - if (xpansion_output_dir.string().find("-Xpansion") == std::string::npos) { - auto log_location = LOGLOCATION; - auto msg = - "Archive path is missing and output path does not contains" - " \"-Xpansion\" suffixe. Can't deduce archive file name."; - throw MismatchedParameters(msg, log_location); - } - auto deduced_archive_path = xpansion_output_dir; - auto dir_name = deduced_archive_path.stem().string(); - dir_name = dir_name.substr(0, dir_name.find("-Xpansion")); - deduced_archive_path = - deduced_archive_path.replace_filename(dir_name).replace_extension( - ".zip"); - return deduced_archive_path; - } - return archive_path; -} -std::filesystem::path ProblemGeneration::deduceXpansionDirIfEmpty( - std::filesystem::path xpansion_output_dir, - const std::filesystem::path& archive_path) { - if (xpansion_output_dir.empty() && !archive_path.empty()) { - auto deduced_dir = archive_path; - deduced_dir = deduced_dir.replace_filename( - deduced_dir.stem().replace_extension("").string() + "-Xpansion"); - return deduced_dir; - } - return xpansion_output_dir; -} struct Version { explicit Version(std::string_view version) { @@ -303,6 +271,3 @@ std::shared_ptr InstantiateZipReader( reader->Open(); return reader; } -ProblemGeneration::MismatchedParameters::MismatchedParameters( - const std::string& err_message, const std::string& log_location) - : XpansionError(err_message, log_location) {} diff --git a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp index 425a30b5b..d65732b86 100644 --- a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp +++ b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp @@ -24,3 +24,41 @@ void ProblemGenerationExeOptions::Parse(unsigned int argc, const char *const *argv) { OptionsParser::Parse(argc, argv); } + +std::filesystem::path ProblemGenerationExeOptions::deduceArchivePathIfEmpty( + const std::filesystem::path& xpansion_output_dir, + const std::filesystem::path& archive_path) const { + if (archive_path.empty() && !xpansion_output_dir.empty()) { + if (xpansion_output_dir.string().find("-Xpansion") == std::string::npos) { + auto log_location = LOGLOCATION; + auto msg = + "Archive path is missing and output path does not contains" + " \"-Xpansion\" suffixe. Can't deduce archive file name."; + throw MismatchedParameters(msg, log_location); + } + auto deduced_archive_path = xpansion_output_dir; + auto dir_name = deduced_archive_path.stem().string(); + dir_name = dir_name.substr(0, dir_name.find("-Xpansion")); + deduced_archive_path = + deduced_archive_path.replace_filename(dir_name).replace_extension( + ".zip"); + return deduced_archive_path; + } + return archive_path; +} + +std::filesystem::path ProblemGenerationExeOptions::deduceXpansionDirIfEmpty( + std::filesystem::path xpansion_output_dir, + const std::filesystem::path& archive_path) const { + if (xpansion_output_dir.empty() && !archive_path.empty()) { + auto deduced_dir = archive_path; + deduced_dir = deduced_dir.replace_filename( + deduced_dir.stem().replace_extension("").string() + "-Xpansion"); + return deduced_dir; + } + return xpansion_output_dir; +} + +ProblemGenerationExeOptions::MismatchedParameters::MismatchedParameters( + const std::string& err_message, const std::string& log_location) + : XpansionError(err_message, log_location) {} \ No newline at end of file diff --git a/src/cpp/lpnamer/main/include/ProblemGeneration.h b/src/cpp/lpnamer/main/include/ProblemGeneration.h index 867a96dbd..f9ac5618f 100644 --- a/src/cpp/lpnamer/main/include/ProblemGeneration.h +++ b/src/cpp/lpnamer/main/include/ProblemGeneration.h @@ -26,15 +26,6 @@ class ProblemGeneration { const std::filesystem::path& log_file_path, const std::filesystem::path& weights_file, bool unnamed_problems); - class MismatchedParameters - : public LogUtils::XpansionError { - using LogUtils::XpansionError::XpansionError; - - public: - explicit MismatchedParameters(const std::string& err_message, - const std::string& log_location); - }; - private: void ProcessWeights( const std::filesystem::path& xpansion_output_dir, @@ -45,10 +36,4 @@ class ProblemGeneration { const std::filesystem::path& antares_archive_path, const std::filesystem::path& xpansion_output_dir, ProblemGenerationLog::ProblemGenerationLoggerSharedPointer logger); - static std::filesystem::path deduceXpansionDirIfEmpty( - std::filesystem::path xpansion_output_dir, - const std::filesystem::path& archive_path); - static std::filesystem::path deduceArchivePathIfEmpty( - const std::filesystem::path& xpansion_output_dir, - const std::filesystem::path& archive_path); }; diff --git a/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h b/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h index a1f0c6711..9f05cdc21 100644 --- a/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h +++ b/src/cpp/lpnamer/main/include/ProblemGenerationExeOptions.h @@ -34,5 +34,12 @@ class ProblemGenerationExeOptions : public OptionsParser, bool UnnamedProblems() const { return unnamed_problems_; } void Parse(unsigned int argc, const char *const *argv) override; + + [[nodiscard]] std::filesystem::path deduceXpansionDirIfEmpty( + std::filesystem::path xpansion_output_dir, + const std::filesystem::path& archive_path) const override; + [[nodiscard]] std::filesystem::path deduceArchivePathIfEmpty( + const std::filesystem::path& xpansion_output_dir, + const std::filesystem::path& archive_path) const override; }; #endif // ANTARES_XPANSION_SRC_CPP_LPNAMER_MAIN_INCLUDE_PROBLEMGENERATIONEXEOPTIONS_H diff --git a/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h b/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h index aa32d223a..e861cf877 100644 --- a/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h +++ b/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h @@ -17,4 +17,19 @@ class ProblemGenerationOptions { [[nodiscard]] virtual std::filesystem::path WeightsFile() const = 0; [[nodiscard]] virtual std::vector ActiveYears() const = 0; [[nodiscard]] virtual bool UnnamedProblems() const = 0; + [[nodiscard]] virtual std::filesystem::path deduceXpansionDirIfEmpty( + std::filesystem::path xpansion_output_dir, + const std::filesystem::path& archive_path) const = 0; + [[nodiscard]] virtual std::filesystem::path deduceArchivePathIfEmpty( + const std::filesystem::path& xpansion_output_dir, + const std::filesystem::path& archive_path) const = 0; + + class MismatchedParameters + : public LogUtils::XpansionError { + using LogUtils::XpansionError::XpansionError; + + public: + explicit MismatchedParameters(const std::string& err_message, + const std::string& log_location); + }; }; diff --git a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp index 89bc2fd5b..207caa1d7 100644 --- a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp @@ -174,7 +174,8 @@ TEST_F( ProblemGenerationSpyAndMock pbg(problem_generation_options_parser_); - EXPECT_THROW(pbg.updateProblems(), ProblemGeneration::MismatchedParameters); + EXPECT_THROW(pbg.updateProblems(), + ProblemGenerationOptions::MismatchedParameters); EXPECT_TRUE(pbg.archive_path_.empty()); // Can't deduce EXPECT_TRUE(pbg.xpansion_output_dir_ diff --git a/tests/cpp/lp_namer/ProblemGenerationTest.cpp b/tests/cpp/lp_namer/ProblemGenerationTest.cpp index 6cd0afce6..b718bc9a0 100644 --- a/tests/cpp/lp_namer/ProblemGenerationTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationTest.cpp @@ -26,6 +26,16 @@ class InMemoryOption : public ProblemGenerationOptions { } std::vector ActiveYears() const override { return std::vector(); } bool UnnamedProblems() const override { return false; } + std::filesystem::path deduceXpansionDirIfEmpty( + std::filesystem::path xpansion_output_dir, + const std::filesystem::path& archive_path) const override { + return std::filesystem::path(); + } + std::filesystem::path deduceArchivePathIfEmpty( + const std::filesystem::path& xpansion_output_dir, + const std::filesystem::path& archive_path) const override { + return std::filesystem::path(); + } }; TEST(InitializationTest, FoldersAreEmpty) { From 9bfb36eee55c35a8d38da6392aa1143cf4790761 Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Fri, 3 Nov 2023 16:45:01 +0100 Subject: [PATCH 08/10] Missing both output and archive path is an error --- .../main/ProblemGenerationExeOptions.cpp | 11 +++++---- .../main/include/ProblemGenerationOptions.h | 14 +++++++++-- .../ProblemGenerationExeOptionsTest.cpp | 24 ++++++++++--------- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp index d65732b86..efe286508 100644 --- a/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp +++ b/src/cpp/lpnamer/main/ProblemGenerationExeOptions.cpp @@ -23,6 +23,11 @@ ProblemGenerationExeOptions::ProblemGenerationExeOptions() void ProblemGenerationExeOptions::Parse(unsigned int argc, const char *const *argv) { OptionsParser::Parse(argc, argv); + if (XpansionOutputDir().empty() && ArchivePath().empty()) { + auto log_location = LOGLOCATION; + auto msg = "Both output directory and archive path are empty"; + throw ProblemGenerationOptions::MissingParameters(msg, log_location); + } } std::filesystem::path ProblemGenerationExeOptions::deduceArchivePathIfEmpty( @@ -57,8 +62,4 @@ std::filesystem::path ProblemGenerationExeOptions::deduceXpansionDirIfEmpty( return deduced_dir; } return xpansion_output_dir; -} - -ProblemGenerationExeOptions::MismatchedParameters::MismatchedParameters( - const std::string& err_message, const std::string& log_location) - : XpansionError(err_message, log_location) {} \ No newline at end of file +} \ No newline at end of file diff --git a/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h b/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h index e861cf877..3fb34b841 100644 --- a/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h +++ b/src/cpp/lpnamer/main/include/ProblemGenerationOptions.h @@ -30,6 +30,16 @@ class ProblemGenerationOptions { public: explicit MismatchedParameters(const std::string& err_message, - const std::string& log_location); + const std::string& log_location) + : XpansionError(err_message, log_location) {} }; -}; + + class MissingParameters : public LogUtils::XpansionError { + using LogUtils::XpansionError::XpansionError; + + public: + explicit MissingParameters(const std::string& err_message, + const std::string& log_location) + : XpansionError(err_message, log_location) {} + }; +}; \ No newline at end of file diff --git a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp index 207caa1d7..eea39cf73 100644 --- a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp @@ -39,17 +39,6 @@ class ProblemGenerationSpyAndMock : public ProblemGeneration { bool unnamed_problems_; }; -TEST_F(ProblemGenerationExeOptionsTest, WithoutArchiveOption) { - char argv = 'c'; - char* pargv = &argv; - char** ppargv = &pargv; - try { - problem_generation_options_parser_.Parse(1, ppargv); - } catch (const std::exception& e) { - EXPECT_EQ(e.what(), - std::string("the option '--archive' is required but missing")); - } -} TEST_F(ProblemGenerationExeOptionsTest, WithoutOutputOption) { const char argv0[] = "lp_namer.exe"; const char argv1[] = "--archive"; @@ -187,4 +176,17 @@ TEST_F( std::ifstream logfile(output_path / "lp" / "ProblemGenerationLog.txt"); EXPECT_TRUE(Constains( logfile, "Archive path is missing and output path does not contains")); +} + +TEST_F(ProblemGenerationExeOptionsTest, + OutputAndArchiveParameters_ErrorIfBothArchiveAndOutputAreMissing) { + auto test_root = + std::filesystem::temp_directory_path() / std::tmpnam(nullptr); + + const char argv0[] = "lp.exe "; + + std::vector ppargv = {argv0}; + + EXPECT_THROW(problem_generation_options_parser_.Parse(1, ppargv.data()), + ProblemGenerationOptions::MissingParameters); } \ No newline at end of file From e32f0b927db4fcd1b361adb19cd50b1ad7aae086 Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Thu, 30 Nov 2023 16:18:08 +0100 Subject: [PATCH 09/10] Fix typo in CMakeLists.txt --- tests/cpp/lp_namer/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cpp/lp_namer/CMakeLists.txt b/tests/cpp/lp_namer/CMakeLists.txt index 619f4f46b..473efc505 100644 --- a/tests/cpp/lp_namer/CMakeLists.txt +++ b/tests/cpp/lp_namer/CMakeLists.txt @@ -35,7 +35,7 @@ add_executable (lp_namer_tests ProblemGenerationLoggerTest.cpp WeightsFileReaderTest.cpp LpFilesExtractorTest.cpp - MpsTxtWriterTest + MpsTxtWriterTest.cpp ProblemGenerationTest.cpp ) From 8126f2de868e97b97ebb86592e74d4cd3710ac5d Mon Sep 17 00:00:00 2001 From: Jason Marechal Date: Thu, 30 Nov 2023 16:40:35 +0100 Subject: [PATCH 10/10] Fix windows build --- tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp index eea39cf73..9be973ae5 100644 --- a/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp +++ b/tests/cpp/lp_namer/ProblemGenerationExeOptionsTest.cpp @@ -98,9 +98,9 @@ TEST_F(ProblemGenerationExeOptionsTest, const char argv0[] = "lp.exe "; const char argv1[] = "--archive"; - auto argv2 = archive; + const auto& argv2 = archive; - std::vector ppargv = {argv0, argv1, argv2.c_str()}; + std::vector ppargv{argv0, argv1, argv2.c_str()}; problem_generation_options_parser_.Parse(3, ppargv.data()); ProblemGenerationSpyAndMock pbg(problem_generation_options_parser_);