jgm/zip-archive

Undeclared dependency on unzip in zip-archive-0.3.2.2

Closed this issue · 19 comments

peti commented

Here we go again ... I report this error for, I dunno, the third time, maybe? Why is that necessary? Do you even care whether this packages compiles in Nix (and other environments with proper sandboxing)?

Anyhow, for the record:

Test suite test-zip-archive: RUNNING...
Cases: 11  Tried: 4  Errors: 0  Failures: 0  adding: LICENSE (deflated 46%)
  adding: src/ (stored 0%)
  adding: LICENSE (deflated 46%)
  adding: src/ (stored 0%)
  adding: src/Codec/ (stored 0%)
  adding: src/Codec/Archive/ (stored 0%)
  adding: src/Codec/Archive/Zip.hs (deflated 74%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/ (stored 0%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/link_to_directory (deflated 13%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/link_to_directory/file.txt (stored 0%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/link_to_file (deflated 14%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/1/ (stored 0%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/1/file.txt (stored 0%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/ (stored 0%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/link_to_directory (deflated 13%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/link_to_file (deflated 14%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/1/ (stored 0%)
  adding: test-zip-archive.1418/test_dir_with_symlinks2/1/file.txt (stored 0%)
Cases: 11  Tried: 6  Errors: 0  Failures: 0  creating: test-zip-archive.1418/dir1
  creating: test-zip-archive.1418/dir1/dir2
 inflating: test-zip-archive.1418/dir1/dir2/hello
extracting: test-zip-archive.1418/dir1/hi
Cases: 11  Tried: 7  Errors: 0  Failures: 0  creating: test-zip-archive.1418/dir3
extracting: test-zip-archive.1418/dir3/hi
Cases: 11  Tried: 10  Errors: 0  Failures: 0/bin/sh: unzip: command not found
### Error in:   10                          
callCommand: unzip ./test-zip-archive.1418/testUnzip.zip (exit 127): failed
Cases: 11  Tried: 11  Errors: 1  Failures: 0
test-zip-archive.1418/test_dir_with_symlinks2/
test-zip-archive.1418/test_dir_with_symlinks2/link_to_directory
test-zip-archive.1418/test_dir_with_symlinks2/link_to_directory/file.txt
test-zip-archive.1418/test_dir_with_symlinks2/link_to_file
test-zip-archive.1418/test_dir_with_symlinks2/1/
test-zip-archive.1418/test_dir_with_symlinks2/1/file.txt
test-zip-archive.1418/test_dir_with_symlinks2/
test-zip-archive.1418/test_dir_with_symlinks2/link_to_directory
test-zip-archive.1418/test_dir_with_symlinks2/link_to_file
test-zip-archive.1418/test_dir_with_symlinks2/1/
test-zip-archive.1418/test_dir_with_symlinks2/1/file.txt
Test suite test-zip-archive: FAIL
Test suite logged to: dist/test/zip-archive-0.3.2.2-test-zip-archive.log

Thanks for reporting the issue @peti

I made the change to test a feature I introduced andunzip is required to test the feature. As there is no list of checks to perform before merging (a part from CI builds on both Linux and Windows) this went unnoticed.

I checked the issues you have previously oped but I was not able to understand what the fix is. I have no experience with Nix packages and lack some other background too probably.

If you would be so kind to propose a fix and suggest a way this can be automatically checked, I would be more than happy to make a PR to implement your suggestions.

Please find the Travis config here: https://github.com/jgm/zip-archive/blob/master/.travis.yml

peti commented

Previous discussion took place in #21, which lead to a fix in #22, which the disappeared again: #35. I never really understood why the build-tools declaration was dropped.

I see, I think input by @jgm is needed here. I'm happy to do the work to bring this back.

How can I test if it's working? Can anything be done to catch this via CI?

peti commented

How can I test if it's working?

Once the Cabal file declares Build-tools: unzip for the test suite, everything will work fine.

Can anything be done to catch this via CI?

Not easily, no.

@peti Is #42 Enough?

peti commented

No. Please take a look at #22.

@peti updated. However now building with stack generates a warning:
Warning: Package zip-archive uses a custom Cabal build, but does not use a custom-setup stanza

peti commented

I have read through your tickets and the linked ones too. Seem everything is in place to me but probably I am missing something, that's all I can gather with my knowledge. I would just like to avoid breaking stackage and hackage again.

jgm commented

@blender you probably need to add a custom-setup stanza to the cabal file, as discussed under #35.

But my previous solution to the problem was to remove the use of 'zip' in the test suite (instead I just packaged a pre-made zip file). We might want to consider just doing the simplest thing here, and just removing the tests you added that require 'zip' and 'unzip'. Yes, these are good tests, but they also make it impossible to build the library on platforms that don't have 'zip' and 'unzip' available (and what about Windows?)

That whole part of the test just runs ifndef _WINDOWS so that is not a problem as shown by the CI.

Sure I can remove the test. It seems to me that they are important though as they test actual compatibility of the archives.

I will add the custom stanza and ping again to see if the problems goes away. Hopefully we won't be breaking anything else.

jgm commented
jgm commented

What was wrong with the idea of adding the custom stanza? That seems much cleaner than the solution proposed in #43.

peti commented

The custom-setup stanza requires cabal-version: >= 1.10, and it's interpreted by version 1.24. Since the Setup.hs script required for declaring build tools does not have any dependencies other than base and Cabal itself, however, old version of Cabal run the build just fine.

great, seems problem is solved then 🎉

jgm commented

Should be fixed in release 0.3.2.3.

peti commented

Yes, the issue is fixed in the new version. We can now derive build instructions for Nix automatically: NixOS/cabal2nix@5ec1188. Thank you very much!

This appears to have broken builds on Windows (#44).