Skip to content

Fix setting archive version#63

Merged
mrkkrp merged 2 commits intomrkkrp:masterfrom
qnikst:master
Mar 11, 2020
Merged

Fix setting archive version#63
mrkkrp merged 2 commits intomrkkrp:masterfrom
qnikst:master

Conversation

@qnikst
Copy link
Copy Markdown
Contributor

@qnikst qnikst commented Mar 9, 2020

This PR fixes setting archive version so unzip tool handles,
permissions set by the library well. In addition in introduces
the helpers for converting unix permissions into the external file
attributes value.

Fixes #62.

@mrkkrp mrkkrp changed the title Fix settings archive version. Fix setting archive version Mar 11, 2020
As per zip spec we may want to encode host version. This version is used by
e.g. the unzip utility in order to understand how to interpret external
info (type of files and permissions).

Current algorithm is pretty naive, we do not test for all OS versions
supported by zip. Instead we check if we run on windows and put 0 in that
case (MS-DOS and OS/2 (FAT / VFAT / FAT32 file systems). Otherwise we put 3
(UNIX).

This way code works properly on the unix OS, and we keep old behavior for
the other OS.

Note that we didn't change ‘toVersion’ function, as it already masked the
higher byte where OS information was stored.

This code was tested manually by storing archive and then unarchiving it via
unzip. There is no automated test because such test would require external
tools (unzip).
@mrkkrp mrkkrp force-pushed the master branch 5 times, most recently from 4399e3c to 840e599 Compare March 11, 2020 19:24
Copy link
Copy Markdown
Owner

@mrkkrp mrkkrp left a comment

Choose a reason for hiding this comment

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

Thanks! Please also see what I had to change. I feel like you went into too many details which not everyone wants to read (normally I want to know as little as possible unless I need to debug it or something). Depending on unix is not necessary in this case, as well as conditional compilation. Changelog entries are a good idea.

It was very hard to set permissions using this library, as the permissions
were not properly documented. Here we try to solve the problem in 2 ways:

1. We add a low level documentation that explains how to work with
   permissions.
2. We introduce unix specific helpers that allow to describe permissions in
   a human readable way.

Unfortunately making those helpers OS agnostic is a pretty hard task we
enable them only on the Unix platforms.
@qnikst
Copy link
Copy Markdown
Contributor Author

qnikst commented Mar 11, 2020

To be honest, having those docs in the package would have saved at least 2 hours of my life. It was not that easy to find relevant documentation that is complete enough. Though I get why you don't want to keep that in package docs.

Wrt System.POSIX.Types, thanks I've missed that they are in base as well.

@mrkkrp
Copy link
Copy Markdown
Owner

mrkkrp commented Mar 11, 2020

To be honest, having those docs in the package would have saved at least 2 hours of my life.

This is understandable, but now we have functions that work as a complete solution to this problem, so explaining the algorithm itself is not necessary.

@mrkkrp mrkkrp merged commit 26b5746 into mrkkrp:master Mar 11, 2020
@mrkkrp
Copy link
Copy Markdown
Owner

mrkkrp commented Mar 11, 2020

Released 1.4.0 with the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Archive version is set incorrectly.

2 participants