Split study in CMakefile #1583

JasonMarechal25 · 2023-08-23T13:58:32Z

Move study in a CMakeFile
Remove dependency to Study in date

…ry_2 # Conflicts: # src/libs/antares/CMakeLists.txt # src/libs/antares/array/CMakeLists.txt # src/libs/antares/benchmarking/CMakeLists.txt # src/libs/antares/checks/CMakeLists.txt # src/libs/antares/correlation/CMakeLists.txt # src/libs/antares/logs/CMakeLists.txt # src/libs/antares/memory/memory.cpp # src/libs/antares/study/memory-usage.h # src/libs/antares/sys/policy.cpp # src/tests/src/libs/antares/CMakeLists.txt

…ry_2

# Conflicts: # src/libs/antares/array/matrix.cpp # src/libs/antares/memory/memory.cpp # src/libs/antares/study/area/area.cpp

# Conflicts: # src/solver/CMakeLists.txt

# Conflicts: # src/analyzer/CMakeLists.txt # src/libs/antares/CMakeLists.txt # src/libs/antares/InfoCollection/CMakeLists.txt # src/libs/antares/benchmarking/DurationCollector.cpp # src/libs/antares/correlation/CMakeLists.txt # src/libs/antares/memory/CMakeLists.txt # src/libs/antares/sys/CMakeLists.txt # src/solver/CMakeLists.txt # src/solver/main/CMakeLists.txt # src/solver/simulation/CMakeLists.txt # src/tools/cleaner/CMakeLists.txt # src/tools/config/CMakeLists.txt # src/tools/kirchhoff-cbuilder/main.cpp # src/tools/updater/CMakeLists.txt # src/tools/yby-aggregator/CMakeLists.txt # src/ui/simulator/CMakeLists.txt # src/ui/simulator/application/main/create.cpp # src/ui/simulator/cmake/application.cmake

flomnes

Add libantares-version ?

Does the build time decrease a bit after this change when editing a deep header, e.g study.h ?

.github/workflows/windows-vcpkg.yml

src/libs/antares/checks/CMakeLists.txt

flomnes · 2023-09-01T13:27:39Z

src/libs/antares/date/date.cpp

+void Calendar::reset(settings settings)
 {
-    reset(parameters, parameters.leapYear);
-}
-
-void Calendar::reset(const Data::Parameters& parameters, bool leapyear)
-{
-    // retrieve the new settings
-    settings.weekday1rstJanuary = parameters.dayOfThe1stJanuary;
-    settings.firstMonth = parameters.firstMonthInYear;
-    settings.weekFirstDay = parameters.firstWeekday;
-
-    // We do not retrieve directly the `leapyear` parameters
-    // A simulation should be made in ignoring this parameter (aka false)
-    // but the outputs should rely on it (for printing).
-    // It goes the same for the GUI : since it is _merely_ printing,
-    // it should be taken into consideration.
-    // Consequently, we will let the calling code specifying this value
-    settings.leapYear = leapyear;
-
-    // re-initialize the calendar with the new settings
+    settings_ = settings;


Pass settings by rvalue-reference ?

Class names come with a capital, so that would be Settings

I don't think r-value ref is proper here. It's a lot of supposition to ask for ownership of a value whereas with pass by value we're signifying that we're doing something to the value without impacting the client.

flomnes · 2023-09-01T13:30:08Z

src/libs/antares/study/CMakeLists.txt

+
+target_include_directories(study
+    PUBLIC
+        ${CMAKE_CURRENT_SOURCE_DIR}/../..


IMO this should avoided just like #include "../../header.hpp"

I've added some comment

#Dirty trick to allow inclusion via <antares/study/X> # Make more than just study visible but it's the lesser evil for now

The proper way would be to have a proper include folder with all study includes (or split study in several library/objects) but it is in itself a task

flomnes · 2023-09-01T13:31:08Z

src/libs/antares/study/load.cpp

+        calendar.reset({parameters.dayOfThe1stJanuary, parameters.firstWeekday, parameters.firstMonthInYear, false});
    else
-        calendar.reset(parameters);
+        calendar.reset({parameters.dayOfThe1stJanuary, parameters.firstWeekday, parameters.firstMonthInYear, parameters.leapYear});

-    calendarOutput.reset(parameters);
+    calendarOutput.reset({parameters.dayOfThe1stJanuary, parameters.firstWeekday, parameters.firstMonthInYear, parameters.leapYear});


Could indeed be passed by &&. But then make sure to mark the copy constructor as deleted

I don't know if this usage should direct the implementation choice of the interface.
For all intent and purpose, parameters could have a Settings struct instead of several values and e would be passing a copy

Yes, that would be nice

src/libs/antares/study/parameters/adq-patch-params.h

src/libs/antares/writer/CMakeLists.txt

flomnes · 2023-09-01T13:38:57Z

src/solver/CMakeLists.txt

+	Antares::date
+	Antares::benchmarking
+	Antares::result_writer
+	Antares::sys
+	Antares::infoCollection
+	Antares::checks


We should avoid heterogeneous naming in CMake targets

infoCollection

result_writer

tests-ts-numbers

In other words, we should pick a convention and stick to it.

See #1606 for discussion

src/tools/cleaner/main.cpp

.github/workflows/windows-release.yml

* Fix merge * Fix compil * Fix compil * Reestablish -j2 build option as it doesn't change anything removing it * Remove commented code * Use proper casing for s(S)ettings struct * Remove duplicated link dependency * add missing dependency in writer

sonarqubecloud · 2023-09-12T10:29:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

14.0% Coverage
0.0% Duplication

JasonMarechal25 and others added 30 commits August 11, 2023 09:52

Move writer in libs

7683adc

wip

f072ec9

Benchmarking

1f54d62

Remove extra dependency

842f75b

Move writer in libs

a7d28cd

Fix post rebase

0b5ac84

Remove Yuni target include

e298249

Lots of lib change

b2cbab5

Lots of lib change

1e6dba1

Remove extra writer dependency

5d3af8f

Improve Cmake correlation

2d2c41a

Improve Cmake checks

de3c1c2

Improve Cmake benchmarking

b1f4a35

Improve Cmake array

23825fe

Improve Cmake exception

9b5ce54

Improve Cmake inifile

cc33857

Improve Cmake io

2aed937

Improve Cmake jit

70cfc64

Improve Cmake logs

2576f12

Fix date cmake

188ae0d

Fix compil for logs

2cc0aa6

Sys wip

5010eb0

Memory wip

2bec0d9

Finish Memory/sys

2c49732

Fix post merge

d8b6d49

Remove extra include

9a81f51

Fix include

7a46828

Remove sys as direct dependency of antares-core (#1561)

45a5f8d

Merge remote-tracking branch 'origin/develop' into feature/cmake_memo…

331d28c

…ry_2

Merge remote-tracking branch 'origin/develop' into feature/work

12ca770

# Conflicts: # src/libs/antares/array/matrix.cpp # src/libs/antares/memory/memory.cpp # src/libs/antares/study/area/area.cpp

pull-request-size bot added the size/XXL label Aug 23, 2023

JasonMarechal25 added 5 commits August 23, 2023 16:05

Merge branch 'feature/cmake_memory_2' into feature/work

1a6f294

# Conflicts: # src/solver/CMakeLists.txt

Merge branch 'feature/work' into feature/study

4fd01cc

Fix merge

d6e5113

Remove lots of libs from core

40b2c65

Remove dependency to libantares-core

d25dc66

Base automatically changed from feature/split_benchmarking to develop August 28, 2023 13:17

JasonMarechal25 and others added 11 commits August 28, 2023 17:00

Fix boost compil

6aacab7

Fix boost compil

cb8b3c3

Fix hostname/policy link error

5df5380

Add missing link to thermal definition

a6d258a

Make yuni public in lib-matrix for test

e397255

Loosen build parallelization

1bb5828

Try fix

f3dd7b5

Remove extra include to log causing link error

26b47b0

Fix windows compilation: missing link to winsock

5905fe4

Fix UI compilation

71fede5

JasonMarechal25 requested review from flomnes, sylvlecl and guilpier-code August 30, 2023 11:17

flomnes requested changes Sep 1, 2023

View reviewed changes

flomnes reviewed Sep 11, 2023

View reviewed changes

.github/workflows/windows-release.yml Outdated Show resolved Hide resolved

JasonMarechal25 added 2 commits September 12, 2023 09:48

Revert removing -j options during CI

28e1e5d

Remove deps to core (#1586)

40e4383

* Fix merge * Fix compil * Fix compil * Reestablish -j2 build option as it doesn't change anything removing it * Remove commented code * Use proper casing for s(S)ettings struct * Remove duplicated link dependency * add missing dependency in writer

flomnes approved these changes Sep 12, 2023

View reviewed changes

flomnes merged commit b6d0993 into develop Sep 12, 2023
7 checks passed

flomnes deleted the feature/study branch September 12, 2023 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split study in CMakefile #1583

Split study in CMakefile #1583

JasonMarechal25 commented Aug 23, 2023 •

edited

Loading

flomnes left a comment •

edited

Loading

flomnes Sep 1, 2023

JasonMarechal25 Sep 1, 2023

flomnes Sep 1, 2023

JasonMarechal25 Sep 1, 2023 •

edited

Loading

flomnes Sep 1, 2023

JasonMarechal25 Sep 1, 2023

flomnes Sep 1, 2023

flomnes Sep 1, 2023

JasonMarechal25 Sep 1, 2023

sonarqubecloud bot commented Sep 12, 2023

Split study in CMakefile #1583

Split study in CMakefile #1583

Conversation

JasonMarechal25 commented Aug 23, 2023 • edited Loading

flomnes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonMarechal25 Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 12, 2023

JasonMarechal25 commented Aug 23, 2023 •

edited

Loading

flomnes left a comment •

edited

Loading

JasonMarechal25 Sep 1, 2023 •

edited

Loading