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.

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

Remove My_zip module #1264

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Remove My_zip module #1264

merged 3 commits into from
Oct 31, 2024

Conversation

Halbaroth
Copy link
Collaborator

@Halbaroth Halbaroth commented Oct 31, 2024

The module My_Zip contains only function which is called once in
Solving_loop. This commit cleans the code and move it in
Solving_loop directly. Technically, Solving_loop is a part of the
binary, but as everyone knows, most of the code of this module will be
push into the library after we design a good API.

This PR replaces #1248

We should use the same test as we only want to check that Alt-Ergo can
unzip correctly the input file if its name has the form:
name.format.zip.
The module `My_Zip` contains only function which is called once in
`Solving_loop`. This commit cleans the code and move it in
`Solving_loop` directly. Technically, `Solving_loop` is a part of the
binary, but as everyone knows, most of the code of this module will be
push into the library after we design a good API.
Copy link
Collaborator

@bclement-ocp bclement-ocp left a comment

Choose a reason for hiding this comment

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

Looks good to me. What are the changes to unzip.ae.zip and unzip.smt2.zip and why are they needed?

@Halbaroth
Copy link
Collaborator Author

Looks good to me. What are the changes to unzip.ae.zip and unzip.smt2.zip and why are they needed?

They did not contain the same test (up to translation). We only want to check that Alt-Ergo can unzip archives and recognize the right format based on the name of the archive. It would be annoying that one of them fails because the zipped test fails.

@bclement-ocp
Copy link
Collaborator

Looks good to me. What are the changes to unzip.ae.zip and unzip.smt2.zip and why are they needed?

They did not contain the same test (up to translation). We only want to check that Alt-Ergo can unzip archives and recognize the right format based on the name of the archive. It would be annoying that one of them fails because the zipped test fails.

Ah yes this is the issue that one is zipped with compression and not the other. We should keep one test for compressed zip and one test for uncompressed zip however. Can we instead rename them to be explicit (e.g. unzip_compressed.ae.zip and unzip_uncompressed.smt2.zip, or the opposite, I don't remember which is which)?

@Halbaroth
Copy link
Collaborator Author

Now, there are both uncompressed:

file unzip.ae.zip 
unzip.ae.zip: Zip archive data, at least v1.0 to extract, compression method=store

and

file unzip.smt2.zip
unzip.smt2.zip: Zip archive data, at least v1.0 to extract, compression method=store

I guess that zip chooses automatically the compression method based on the size of the file. Compressing a tiny file will produce a table larger than the file itself, so zip chooses the store method.

If you want, I can force the method with the option -Z, but I think it does not matter.

@bclement-ocp
Copy link
Collaborator

Please do keep a test with a compressed zip file. It is a use case we want to support so it should be in the test suite.

@Halbaroth
Copy link
Collaborator Author

Done. The option -Z cannot force zip to compress file. So I added a padding comment in both tests to ensure that they will be compressed.

Add a padding comment in both tests to ensure that zip won't choose
the store method while compressing them.
@Halbaroth Halbaroth merged commit 9f5dbd7 into OCamlPro:next Oct 31, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants