Feat/zip features by cutullic · Pull Request #139 · SolarFramework/SolARFramework

cutullic · 2026-09-02T15:07:55Z

No description provided.

jim-bcom · 2026-09-02T15:28:51Z

+    try {
+        // Create a working directory for zip/unzip features
+        fs::path wp(m_workingPath);
+        if (!fs::exists(wp)) {


Use temp_directory_path https://en.cppreference.com/cpp/filesystem/temp_directory_path ?

This way you are guaranteed to get an existing directory, and it might enventually be removed when the machine restarts, if for some reason you were not able to delete it sooner.

Here, if the directory does exists, you might use it, and potentially overwrite user data, since it's not handled as an error.

Now that I think of it, concerning the two PR comment this seems to address: you might want to split the RAII part you did with the dtor of ZipBuffer, from the utility methods.

Make ZipBufferUtils (or whatever :)) have these 2 methods of extraction/compression as static helpers

ZipBufferUtils::extract() ZipBufferUtils::create/compress/...()

create a RAII object responsible to create the tmp dir and deleting it on destruction, something like:

class ScopedTmpDir { ScopedTmpDir () { m_path = temp_directory_path();} ~ScopedTmpDir { fs::remove_all(m_path ); } fs::path getPath(); };

And use it in the static helper function.

This adds the benefit of deleting things ASAP, instead of waiting that the ZipBuffer object goes out of scope, which may be long after the end of the function (which I gues is why you kept deleting things at the end of the methods, instead of relying on the destructor, as I commented elsewhere).

What do you think of this approach ?

I like this approach :-) Made the modifications.

jim-bcom · 2026-09-02T15:32:44Z

+    try {
+        // Check working directory
+        fs::path wp(m_workingPath);
+        if (!fs::is_directory(wp)) {


If temp_directory_path this check is probably not needed (even in the current version). Unless we fear something might happen to this directory between the creation of ZipBuffer and the call to this.

Removed because temp_directory_path is now used.

jim-bcom · 2026-09-02T15:34:30Z

+        fs::path op(originalPath);
+        if (!fs::is_directory(op)) {
+            LOG_ERROR("The original path is not a directory: {}", originalPath);
+            return FrameworkReturnCode::_NOT_FOUND;


Maybe !exists => NOT_FOUND but !directory => ERROR (if we don't have BAD_ARGUMENT) ?

jim-bcom · 2026-09-02T15:35:45Z

+        }
+        if (fs::is_empty(op)) {
+            LOG_ERROR("The original path is empty: {}", originalPath);
+            return FrameworkReturnCode::_NOT_FOUND;


SUCCESS? You give an empty dir, you get an empty vector, job done ?

OK returns SUCCESS but with a warning (and clear the input buffer)

jim-bcom · 2026-09-02T15:42:05Z

+
+        // Copy data to zip in the working directory
+        const auto copyOptions = fs::copy_options::recursive;
+        fs::copy(op, wp, copyOptions);


Can't we avoid the copy ? For example by invoking zip directly from originalPath ?

std::string command = "cd " + originalPath + ";zip -r " + m_workingPath+ "/data.zip .";

No, I prefer not to work on the original directory, to ensure I don't modify it.

Ok, but I'm not sure I see what you fear of invoking zip, if you specify a different destination dit for the zip file, it shouldn't touch the origin one.

It's just that it might be costly to copy a large amount of data. The current implementation is already not ideal performance wise (using a temporary zip file on disk i.o. zipping it in memory, which produces a lot of I/O), and this adds up.

OK we'll see if it becomes a bottleneck.

OK, I see the point. I will try to change this behavior.

jim-bcom · 2026-09-02T15:53:33Z

+            fs::remove(zipFile);
+
+            // Copy unzipped data in the destination directory
+            const auto copyOptions = fs::copy_options::recursive;


Could we avoid the copy by invoking zip from the destination directory?

std::string command = "cd " + destinationPath+ "; unzip " + m_workingPath + " /data.zip";

See previous comment

jim-bcom · 2026-09-02T15:53:49Z

+
+// private
+
+void ZipBuffer::cleanWorkingDirectory()


No longer useful at all: removed

jim-bcom · 2026-09-02T16:01:02Z

+    /// * FrameworkReturnCode::_SUCCESS if the process succeeds
+    /// * FrameworkReturnCode::_NOT_FOUND if data is not found in original path
+    /// * else FrameworkReturnCode::_ERROR_
+    FrameworkReturnCode zipToBuffer(const std::string & originalPath,


nit: use directly fs::path i.o. string for path ?

I thought using std::string avoided forcing the caller to use std::filesystem?

jim-bcom · 2026-09-02T16:03:33Z

+    /// @return
+    /// * FrameworkReturnCode::_SUCCESS if the process succeeds
+    /// * else FrameworkReturnCode::_ERROR_
+    FrameworkReturnCode bufferToUnzip(const std::vector<unsigned char> & compressedZipBuffer,


nit: matter of taste, but with verbs for method ? ZipBuffer::extract()/ZipBuffer::compress()? Or something else.

jim-bcom · 2026-09-02T16:07:21Z

+ *
+ */
+
+class ZipBuffer {


Again naming things is hard... It does not per say represent a ZipBuffer, but more like utils functions... But I'm not sure about ZipBufferUtils... I don't have a better idea :). Something like ZipBufferExtractor (but it does also compress, so ...), or ZipBufferProcessor,...

What about just ZipUtils ?

Let's go for ZipBufferUtils class and extractand compressmethods

jim-bcom · 2026-09-03T12:05:57Z

+#include <vector>
+#include <filesystem>
+
+namespace fs = std::filesystem;


Remove, avoid aliasing in header, especially at this scope.

Yep, sorry!

jim-bcom · 2026-09-03T12:14:09Z

+     * @brief <B>Create a temporary working directory</B>
+     *
+     */
+    class ScopedWorkingDir {


This class is decorrelated to ZipBufferUtils, so 2 suggestions:

define in its own file for anyboady to reuse

define it in the cpp file, because it's not referenced here, so it does not have to be defined here.

It's only an implementation detail that does not have to surface here (except if this class is needed elsewhere, but then this would have to be defined in its own file, as suggested in 1.

Also maybe keep Tmp or Temp in the name (ScopedTmpDir), so that we know it uses the tmp folder API, and this is where the file is created ...?

jim-bcom · 2026-09-03T12:24:21Z

+    class ScopedWorkingDir {
+    public:
+        ScopedWorkingDir() { m_workingPath = fs::temp_directory_path(); m_workingPath += "/solar"; }
+        ~ScopedWorkingDir() { fs::remove_all(m_workingPath); }


Delete copy operations to prevent double deletion

ScopedWorkingDir(const ScopedWorkingDir&) = delete; ScopedWorkingDir& operator=(const ScopedWorkingDir&) = delete; ScopedWorkingDir(ScopedWorkingDir&&) = delete; ScopedWorkingDir& operator=(ScopedWorkingDir&&) = delete;

(move operations are already deleted by the fact of defining the destructor, but this way it's explicit)

jim-bcom · 2026-09-03T12:30:14Z

+     */
+    class ScopedWorkingDir {
+    public:
+        ScopedWorkingDir() { m_workingPath = fs::temp_directory_path(); m_workingPath += "/solar"; }


Make the subdir a (optional?) parameter of the (explicit) ctor?

explicit ScopedWorkingDir(const std::string& subDir = "");

This class has moved to the .cpp file, and will only be used by ZipBufferUtils static methods, so I don't think it's usefull to make the sudir configurable

jim-bcom · 2026-09-03T12:32:10Z

+     */
+    class ScopedWorkingDir {
+    public:
+        ScopedWorkingDir() { m_workingPath = fs::temp_directory_path(); m_workingPath += "/solar"; }


nit: use operator / for path separator, example of syntax:

m_workingPath = m_workingPath / "solar"; m_workingPath /= "solar";

Both operations are available : why this one instead of the other one?

OK, changed

jim-bcom · 2026-09-03T12:58:38Z

+
+        // Copy data to zip in the working directory
+        const auto copyOptions = fs::copy_options::recursive;
+        fs::copy(op, wp, copyOptions);


Ok, but I'm not sure I see what you fear of invoking zip, if you specify a different destination dit for the zip file, it shouldn't touch the origin one.

It's just that it might be costly to copy a large amount of data. The current implementation is already not ideal performance wise (using a temporary zip file on disk i.o. zipping it in memory, which produces a lot of I/O), and this adds up.

OK we'll see if it becomes a bottleneck.

jim-bcom · 2026-09-03T13:01:08Z

+        LOG_DEBUG("ZipBufferUtils::extract - Working temporary path: {}", workingDir.getStringPath());
+
+        // Check/create the working directory
+        if (!fs::exists(workingDir.getPath())) {


No longer needed?

Yes, still needed here: the subdir must be created here before creating the zip file (tested)

Since last commit, ScopedTempDir creates the directory, so block l.127-132 is no longer needed, right?

Yes, right, removed this part

jim-bcom · 2026-09-03T13:03:19Z

+    }
+
+    try {
+        fs::path dp(destinationPath);


In the other function you clear the output buffer, here we could delete the content of the destination path as well ?

I don't think so: this destination directory can contain other files, we don't know.

jim-bcom · 2026-09-03T13:04:24Z

+            return FrameworkReturnCode::_ERROR_;
+        }
+
+        // Delete the zip file


No longer needed ?

Ah yes you do it because the zip is in the same dir as the one you copy later on, OK.

jim-bcom · 2026-09-03T13:06:30Z

+        file.close();
+
+        // Try to unzip the file content
+        std::string command = "cd " + workingDir.getStringPath() + "; unzip data.zip";


Here you could avoid the copy by cd in the dest dir, there's really no risk of messing with existing files, since it's supposed to receive ours, no?

Yes, I think you're right and I will change it

…projects.

…or changes

jim-bcom · 2026-09-08T09:00:00Z

+     * @brief <B>Create a temporary directory</B>
+     *
+     */
+class ScopedTempDir {


In its own file ? It's not related to zip buffer.

I prefer not to create one more file. What do you think?

Well, I tend to agree with my first comment obviously :) ...

This has nothing to do with ZipBuffer, it's an util class that can be used in any other context, so for me it should be separated from ZipBuffer. I know the class is defined outside the ZipBufferUtils class itself, but it seems odd to have to include ZipBufferUtils.h to be able to create a tmp dir.

Also, I did not notice, but maybe these could also defined in a dedicated namespace like SolAR::util, SolAR::io, or SolAR::util::io, I don't know.

OK, I will create new files for ScopedTempDir and put this 'util' classes in SolAR::util

jim-bcom · 2026-09-08T09:00:34Z

+    ScopedTempDir(ScopedTempDir&&) = delete;
+    ScopedTempDir& operator=(ScopedTempDir&&) = delete;
+
+    const std::filesystem::path getPath() const;


nit: std::filesystem::path&

jim-bcom · 2026-09-08T09:24:17Z

+ScopedTempDir::ScopedTempDir(const std::string &subdirectory)
+{
+    m_tempPath = fs::temp_directory_path();
+    m_tempPath /= subdirectory;


We should ensure this directory does not already exist. When I first suggested the use of temp_directory_path I too quickly assumed it was similar to mktemp command but it's actually not.

What we want is an equivalent of mktemp -d

Maybe take a look at tmpnam, but it's not required to be thread safe, so there might be a name conflict, but it's still better than the current situation.

We could also name it with the current timestamp (again, not bullet proof, but maybe good enough for now)

This way you can either remove the subdirectory parameter, or make it optional (with a default value of ""), so that the directory name can have a human readable substring to identify its purpose (e.g. /tmp/compress_6985678986865)

Update: avoid using tmpnam, and build a unique file name.

you can do something like this with random number generator:

fs::path base_path = fs::temp_directory_path(); std::random_device rd; std::mt19937_64 gen(rd()); std::uniform_int_distribution<uint64_t> dis; do { std::string random_name = "tmp_" + std::to_string(dis(gen)); m_tempPath = base_path / random_name; } while (fs::exists(m_tempPath )); std::error_code ec; fs::create_directories(m_tempPath, ec);

(or use timestamp I suggested above, same idea)

NIce: I test it

It works well: I am making the changes.

jim-bcom · 2026-09-08T09:25:37Z

+
+public:
+
+    ScopedTempDir() = delete;


Is this needed ? If you define the ctor with one parameter, the class no longer has a default ctor no?

You are sure?

Irrelevant given the latest changes: no parameters for the constructor.

jim-bcom · 2026-09-08T09:35:34Z

+    /// @return
+    /// * FrameworkReturnCode::_SUCCESS if the process succeeds
+    /// * else FrameworkReturnCode::_ERROR_
+    static FrameworkReturnCode extract(const std::string & destinationPath,


I just saw the fix you did in MapManager where you inverted the parameter. I didn't notice it, but maye it is indeed more intuitive to change the order of the parameters between the two functions, and keep order as (origin -> destination):

compress(path, compressedZipBuffer); extract(compressedZipBuffer, path);

Yes, I think you're right: I'll come back to that.

cutullic added 2 commits September 2, 2026 14:53

feat: add new class ZipBuffer to manage zip/unzip data to/from buffer

a8470a8

feat: ZipBuffer methods return FrameworkReturnCode instead of boolean

1fbeac7

cutullic requested a review from jim-bcom September 2, 2026 15:07

jim-bcom reviewed Sep 2, 2026

View reviewed changes

refactor: changes following PR comments

d8c8c46

jim-bcom reviewed Sep 3, 2026

View reviewed changes

cutullic added 3 commits September 3, 2026 18:27

refactor: changes following PR comments

8a50ced

feat: expose the ScopedTempDir class so that it can be used by other …

e4935e8

…projects.

feat: add a subdirectory in the ScopedTempDir constructor + other min…

2689a48

…or changes

jim-bcom reviewed Sep 8, 2026

View reviewed changes

cutullic added 2 commits September 8, 2026 14:34

feat: generate a random name for temporary subdirectory

6cc1c26

feat: put ScopedTempDir class in a separate file + SolAR::util namespace

e516bf7

Uh oh!

Conversation

cutullic commented Sep 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!