Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#810 Add the possibility to zip the inputs and outputs for N simulation #820

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

#810 Add the possibility to zip the inputs and outputs for N simulation #820

wants to merge 30 commits into from

Conversation

barondim
Copy link
Collaborator

Checklist before requesting a review

use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes

  • unit tests and non regression tests were added (update of inputs, outputs, SA, algos)
  • main documentation was updated (update of input/output file)
  • the corresponding milestone was added in the ticket and in this PR

@barondim barondim added this to the v1.7.0 milestone Dec 20, 2024
@barondim barondim marked this pull request as ready for review December 20, 2024 19:02
@barondim barondim force-pushed the 810_SA_add_the_possibility_zip_the_inputs_and_outputs branch from cc022b3 to abe282a Compare December 20, 2024 19:06
@barondim barondim linked an issue Jan 6, 2025 that may be closed by this pull request
[SA] add the possibility zip the inputs and outputs #810
Open
@barondim barondim force-pushed the 810_SA_add_the_possibility_zip_the_inputs_and_outputs branch from abe282a to 1826950 Compare January 6, 2025 15:26
rosiereflo
rosiereflo requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@rosiereflo rosiereflo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the N output will not be zipped. Also, the documentation and tests needs to be updated.

scripts/envDFL.sh Show resolved Hide resolved
@barondim
#810 Handle libzip error
efc0b5b 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim barondim requested a review from rosiereflo January 9, 2025 10:48
rosiereflo
rosiereflo reviewed Jan 9, 2025
View reviewed changes
sources/Common/src/ZipErrorMessage.cpp


std::string formatZipErrorMessage(const zip::ZipException& e) {
std::stringstream zipErrorStream;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why overriding the messages sent by libzip? The general exception catch should be enough ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message given by libzip is not always clear. When the zip::Error::LIBARCHIVE_INTERNAL_ERROR error code is thrown, the message is clear : ''Unrecognized archive format". But when the zip::Error::FILE_NOT_FOUND error code is thrown, only the name of the missing file is given. So the error message is not clear in that case.

@barondim barondim force-pushed the 810_SA_add_the_possibility_zip_the_inputs_and_outputs branch from c1922bc to 852124a Compare January 14, 2025 18:36
@barondim barondim force-pushed the 810_SA_add_the_possibility_zip_the_inputs_and_outputs branch 2 times, most recently from f84dc5c to 34332f4 Compare January 15, 2025 11:45
rosiereflo
rosiereflo reviewed Jan 17, 2025
View reviewed changes
sources/Context.cpp Outdated Show resolved Hide resolved
@barondim barondim force-pushed the 810_SA_add_the_possibility_zip_the_inputs_and_outputs branch from 64a3d70 to c54c1dd Compare January 17, 2025 10:06
@barondim barondim mentioned this pull request Jan 17, 2025
Merged
6 tasks
@barondim
Copy link
Collaborator Author

@barondim
#810 Little fix
0c01277 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Handle the case when the simulation doesn't dump final state IID…
5020b43 
…M output file

Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Doxygen
a6ef06e 
Signed-off-by: Dimitri Baron <[email protected]>
rosiereflo
rosiereflo reviewed Jan 17, 2025
View reviewed changes
sources/Context.cpp Outdated Show resolved Hide resolved
rosiereflo
rosiereflo reviewed Jan 17, 2025
View reviewed changes
sources/main.cpp Outdated Show resolved Hide resolved
@barondim
#810 Add a specific error if Log::addLogFileContentInMapData() method…
1cac7e3 
… can not open the input log file

Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Do not add data in mapData if the zip mode is not activated
32a042f 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Postponing results.json generation is useless
57033a1 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 PR remark : check that the zip archive exists in envDFL.sh
16b413e 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Renaming + doxygen
633a6e2 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Deletion of ZipErrorMessage.cpp
5390c96 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Renaming
3b05720 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Populate the output files map even if the simulation crashes
df7990a 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Check which outputs should be generated in Context::populateOutp…
00a6950 
…utsMapWithSimulationOutputs()

Signed-off-by: Dimitri Baron <[email protected]>
@barondim barondim force-pushed the 810_SA_add_the_possibility_zip_the_inputs_and_outputs branch from 4c87ee3 to 00a6950 Compare January 20, 2025 15:46
@barondim barondim requested a review from rosiereflo January 20, 2025 18:23
@barondim
#810 Ensure the files are removed if they already exist before unzipp…
de3c5da 
…ing the archive

Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Documentation
880fa95 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Override libzip error message
e87f470 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Check whether all required files are present in the specified ar…
4b56c67 
…chive

Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Little bug fix
36f5773 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Little bug fixes
199b017 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 Renaming + add a unit test on checking missing files in archive
0aef6cb 
Signed-off-by: Dimitri Baron <[email protected]>
@barondim
#810 force flush in dynawo.log
1ea2e32 
Signed-off-by: Dimitri Baron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcochiaramello marcochiaramello Awaiting requested review from marcochiaramello

@rosiereflo rosiereflo Awaiting requested review from rosiereflo

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.7.0
Development

Successfully merging this pull request may close these issues.

[SA] add the possibility zip the inputs and outputs
2 participants
@barondim @rosiereflo