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

Some plugins don't handle truncated compressed buffers correctly #204

Open
3 of 5 tasks
nemequ opened this issue Jun 21, 2016 · 10 comments
Open
3 of 5 tasks

Some plugins don't handle truncated compressed buffers correctly #204

nemequ opened this issue Jun 21, 2016 · 10 comments
Milestone

Comments

@nemequ
Copy link
Member

nemequ commented Jun 21, 2016

I've added a /bounds/decode/truncated test (b15170a), several codecs fail:

@nemequ nemequ added this to the 0.8 milestone Jun 21, 2016
@rmax
Copy link

rmax commented Feb 12, 2017

I'm trying to build a conda[1] package[2] for this awesome library, but for 0.7 release I get a failure for BSC in OSX. See https://travis-ci.org/conda-forge/staged-recipes/builds/200927827#L1496

Is it expected to some plugins fail under some tests? I've seen ms-compress fail under threads tests too.

[1] https://conda.io/
[2] conda-forge/staged-recipes#2348

@nemequ
Copy link
Member Author

nemequ commented Feb 12, 2017

We didn't have CI set up for OS X (or Windows, IIRC) until after 0.7 was released, so it's definitely possible there were bugs. We have found, reported, and often fixed, a lot of bugs in various libraries. I know that ms-compress and bsc have both fixed bugs we've reported, but honestly I'm not sure if that was before or after 0.7 was released (or both).

make test shouldn't fail. If a test is failing and we want to ignore it we skip the test, which is what we do for the plugins mentioned in the first comment of this issue.

TBH I'm pretty focused on 0.8 right now (I still have a lot of stuff I want to do before releasing it, and it's long overdue), but we could probably put out a 0.7.1 release with many of the plugins updated and any other bug fixes you need. I don't have an Apple machine, so any help here would be appreciated.

I've never heard of Conda before, but it sounds interesting… I'm not sure how the packages work, but as long as someone is willing to help maintain it I'm willing to integrate packaging (and bindings) into the Squash repo so it's tested in our CI before it is pushed to master.

@rmax
Copy link

rmax commented Feb 13, 2017

Hey @nemequ, thanks for your prompt response.

Conda is a multi-platform package manager that allows to manage multiple "virtual" environments with binary packages. Conda-Forge[1] is a community effort to provide additional packages to the conda ecosystem. It's similar to Homebrew, where you have recipes that specify how to build a particular package, but the distributions is in binary format.

Cool thing about Conda-Forge is that provide an integrated release and build management via GitHub and free CI services; Travis, Appveyor, CircleCI, one for each platform OSX, Windows, Linux, respectively.

The conda recipe PR is here: conda-forge/staged-recipes#2348
and I collected these patches: https://github.com/rolando-contrib/staged-recipes/tree/squash-recipe/recipes/squash/patches
which mainly address OSX compilation issues. But I'm a bit lost about how to fix the bsc/ms-compress segfaults, and I don't know what these failures imply for end-users usage.

For now @jakirkham (from conda-forge) has agreed that we can skip the tests selectively in order to have an OSX release.

If you see we can easily wrap up a 0.7.1 release, I'm happy to help you. Otherwise, we can wait for the 0.8 release. Nevertheless, after we get accepted the 0.7 conda recipe I plan to continue with a 0.8-dev recipe so we can already test the build on OSX and Linux.

PD: I started all of this because I wanted to play with different compression algorithms and squash provides a convenient abstraction API. I also plan to write CFFI-based Python bindings.

[1] https://conda-forge.github.io/

@nemequ
Copy link
Member Author

nemequ commented Feb 13, 2017

Cool thing about Conda-Forge is that provide an integrated release and build management via GitHub and free CI services; Travis, Appveyor, CircleCI, one for each platform OSX, Windows, Linux, respectively.

Can you build local packages, too, though? My thinking is that if we build one for each Squash commit it could help avoid any surprises down the line. That said, it's probably not a big deal… these days we have CI running for Linux and OS X (Travis) and Windows (AppVeyor), so 0.8+ should be much more reliable.

But I'm a bit lost about how to fix the bsc/ms-compress segfaults, and I don't know what these failures imply for end-users usage.

For a (possible) 0.7.1 release, my thinking is to just update all the submodules to pull in the latest version of each compression library. A few libraries (like brotli) have made API changes which would make this harder, but we could either skip those or backport the changes in the Squash plugins.

Since we started using CI on OS X and Windows we've found a lot of bugs which have been fixed upstream, including in BSC and ms-compress. We've also improved our tests which helped us nail down some sporadic failures, and ASan and UBSan have helped us find some more issues. Basically, there is a good chance that simply pulling in the latest versions of some of the submodules will fix the segfaults.

If you see we can easily wrap up a 0.7.1 release, I'm happy to help you. Otherwise, we can wait for the 0.8 release. Nevertheless, after we get accepted the 0.7 conda recipe I plan to continue with a 0.8-dev recipe so we can already test the build on OSX and Linux.

Honestly, I'm not sure how easy a decent 0.7.1 release would be. Updating a few submodules isn't a big deal, but there were changes in Squash, too…

One thing to keep in mind is that 0.8 is an API break. I don't know how Conda handles that, but for Linux distros the preferred solution would be to have squash-0.7 and squash-0.8 be separate packages. Once 1.0 is out the API will be stable until 2.0, but I'm trying to get all the breaks done with 0.8 and hopefully 0.9 and 1.0 will not need any.

PD: I started all of this because I wanted to play with different compression algorithms and squash provides a convenient abstraction API. I also plan to write CFFI-based Python bindings.

Nice! If you do create Python bindings I'd definitely like to integrate them into the main repo so we can integrate them with CI to make sure I don't break anything.

@jakirkham
Copy link

Can you build local packages, too, though? My thinking is that if we build one for each Squash commit it could help avoid any surprises down the line. That said, it's probably not a big deal… these days we have CI running for Linux and OS X (Travis) and Windows (AppVeyor), so 0.8+ should be much more reliable.

At conda-forge, our goal is to help make proper (tagged) releases available to users. However, we don't aim to supplant good development practices in the primary code base itself (e.g. using CIs). Hope that makes sense.

@rmax
Copy link

rmax commented Feb 13, 2017

Can you build local packages, too, though?

Yes. You can build your own packages and host it yourself or in anaconda.org. Conda-forge is set up for official releases, though.

Honestly, I'm not sure how easy a decent 0.7.1 release would be. Updating a few submodules isn't a big deal, but there were changes in Squash, too…

I think we can wait for users' demand. If there are conda users that have issues with the 0.7.0 release then we can think of a 0.7.1 release.

Conda itself doesn't do anything to avoid API break. If two releases for the same package share the same paths, then you can create two conda environments, one for each. But if the releases use proper versioning then it will work just fine in conda. For instance, squash headers are installed under PREFIX/include/squash-0.7 and the library as PREFIX/lib/libsquash0.7.dylib.

Also pkg-config returns the correct values:

$ pkg-config --cflags squash-0.7
-I/Users/rolando/miniconda3/envs/myenv/include/squash-0.7 
$ pkg-config --libs squash-0.7
-L/Users/rolando/miniconda3/envs/myenv/lib -lsquash0.7

Regarding the Python binding, my plan is to build squash statically and vendor the library in the python package. In that way won't have to worry about installing the correct squash version (regardless they use conda or not).

Sorry for hijacking this issue!

@nemequ
Copy link
Member Author

nemequ commented Feb 13, 2017

Multiple versions of the library are parallel-installable, though the squash executable isn't…

Regarding the Python binding, my plan is to build squash statically and vendor the library in the python package. In that way won't have to worry about installing the correct squash version (regardless they use conda or not).

You may be able to get a static version of libsquash built, but it may not do much good. You can't roll the plugins into libsquash, so you still need to make sure the right version of Squash is installed if you want to actually do anything with libsquash. Even if it were technically possible (it would require some changes), there are licensing issues which pretty much preclude it.

Static plugins would mess up the ability to add additional plugins… there are already a couple plugins in the squash-plugins-extra repo, and hopefully in the future third parties will be able to distribute plugins (especially important for proprietary libraries).

Sorry for hijacking this issue!

No worries, I'm just happy to be making progress on this :). We should probably move future Python talk to #34, though.

@jakirkham
Copy link

FWIW we also support dev releases at conda-forge. So that is something we can explore. Just every commit is not practical in that context. Another thing you could try if building every commit is something you want to do is to setup your own feedstock here using conda-smithy and configure it to upload to your own channel. This could be loosely based off the feedstock in conda-forge.

@nemequ
Copy link
Member Author

nemequ commented Feb 13, 2017

I wasn't trying to suggest that we upload a package for every commit, merely that we build one. Basically, using CI to verify that we don't accidentally break the packaging… I hate the idea of relying on patches to fix issues.

However, if the main concern is just about being able to build squash on those platforms as opposed to being able to build the squash conda package, I'm not as concerned since we have a pretty good CI system setup now, including for platforms I don't use.

@jakirkham
Copy link

Ah ok. Yeah, conda-build scripts are thin wrappers over whatever build is occurring underneath the hood (e.g. make, nmake, cmake, etc.) with some post-build tests. As it sounds like you have CIs for the 3 platforms we support, I agree that we should be ok.

Though if you do want to starting get dev releases out for user to try, we are happy to collaborate with you at conda-forge to make that happen.

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

No branches or pull requests

3 participants