Add snappy #193

groutr · 2016-03-23T21:01:57Z

Add Google's snappy library
https://github.com/google/snappy

jakirkham · 2016-03-23T21:12:12Z

🎉 Thanks @groutr. This is awesome. Will take a closer look when I'm at my computer.

ocefpaf · 2016-03-23T21:12:39Z

recipes/snappy/meta.yaml

+  home: https://github.com/google/snappy
+  license: Other
+  license_file: COPYING
+  license_family: Other


Needs maintainer.

groutr · 2016-03-23T21:22:40Z

I have a pre-generated configure script. Do we want to include that in the recipe rather than rely on autogen.sh?

jakirkham · 2016-03-24T04:31:57Z

Phew, that feels like a lot in that last commit!

What's wrong with autogen? We also have automake and autoreconf tools here that you can use for this.

groutr · 2016-03-26T00:02:25Z

There's nothing wrong with autogen. It just seemed that the circleci builds were failing because of it.

conda-forge-admin · 2016-03-30T16:30:56Z

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found some lint.

Here's what I've got...

For recipes/snappy:

The summary item is expected in the about section.
The recipe must have some tests.

groutr · 2016-03-30T16:31:41Z

Do all the CI machines have autotools? If so, I'll revert the commit of all the files that autotools generates.

jakirkham · 2016-03-30T16:36:13Z

Do all the CI machines have autotools?

Meaning autoconf, automake, m4, etc.? For UNIX systems, yes, we have these. Not for Windows though (there may be exceptions).

This reverts commit 0a08730.

conda-forge-admin · 2016-03-30T16:45:28Z

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found some lint.

Here's what I've got...

For recipes/snappy:

The summary item is expected in the about section.
The recipe must have some tests.

jakirkham · 2016-03-30T17:21:47Z

We also have cmake if that helps.

groutr · 2016-03-30T18:38:46Z

autogen.sh fails becase:

aclocal is missing on circleci
libtoolize is missing on travis

snappy doesn't use cmake.

ocefpaf · 2016-03-30T18:44:32Z

autogen.sh fails becase:

aclocal is missing on circleci

libtoolize is missing on travis

We have libtool package for OS X and Linux. That should solved it, no?

jakirkham · 2016-03-30T18:46:01Z

Also, aclocal is part of automake, which we have. Agreeing with @ocefpaf please make these build time dependencies on UNIX only.

groutr · 2016-03-30T18:57:55Z

Oh, I expected those the come with the CI, not be dependencies. I can change the recipes.

conda-forge-admin · 2016-03-30T18:59:56Z

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found some lint.

Here's what I've got...

For recipes/snappy:

The summary item is expected in the about section.
The recipe must have some tests.

jakirkham · 2016-03-30T19:01:11Z

Yeah, we are getting pretty low level here. Linux is a bare docker container with a compiler and a few X11 dependencies so that Qt can survive. Mac has brew, but we are planning on removing just about everything from it.

jakirkham · 2016-03-30T19:06:14Z

recipes/snappy/bld.bat

+REM Build step
+devenv "%SLN_FILE%" /Build "%SLN_CFG%|%SLN_PLAT%"
+if errorlevel 1 exit 1
+


Drop extra newline.

jakirkham · 2016-03-30T19:08:17Z

Think you need pkg-config too. We also have this.

jakirkham · 2016-03-30T19:09:47Z

recipes/snappy/meta.yaml

+  build:
+    - msinttypes  # [win]
+    - libtool # [unix]
+    - automake # [unix]


Add pkg-config for Unix.

Also, selectors.

2 spaces after package name (minimum)

line up indentation of these selectors.

jakirkham · 2016-03-30T19:11:11Z

Think we're close. Couple more things noted above.

conda-forge-admin · 2016-03-30T23:54:45Z

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found some lint.

Here's what I've got...

For recipes/snappy:

The recipe must have some tests.

conda-forge-admin · 2016-03-31T00:22:57Z

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found some lint.

Here's what I've got...

For recipes/snappy:

The recipe must have some tests.

conda-forge-admin · 2016-03-31T00:24:26Z

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found some lint.

Here's what I've got...

For recipes/snappy:

The recipe must have some tests.

conda-forge-admin · 2016-03-31T00:26:31Z

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found some lint.

Here's what I've got...

For recipes/snappy:

The recipe must have some tests.

jakirkham · 2016-03-31T04:25:11Z

As for tests, could you please just verify the relevant libraries are installed? If there happen to be executables that ship with it, checking version or help should be sufficient. This will also make the linter happy.

jakirkham · 2016-03-31T04:27:31Z

recipes/snappy/meta.yaml

+  version: {{ version }}
+
+source:
+  git_url: https://github.com/google/snappy.git


Could we just download the tarball instead? Should be faster and may avoid a few extra configuration steps.

Are tarballs preferred over repositories?

IMHO, yes, for many reasons:

download size is (probably) smaller

tarballs + sha/md5 is a stricter definition of version than a git tag (tags can change to other revisions)

Also, they may have run autoconf and other things so it is simpler to build. With many google libraries the standard has been downloading gtest and gmock, though they may have tarballs where this is already done.

groutr · 2016-04-01T15:58:23Z

@jakirkham I dug up some old PRs and saw how they do tests. I'll get that in the next couple of commits.
Currently, I'm trying to figure out why this recipe is failing for windows.

jakirkham · 2016-04-01T16:08:05Z

Here's an ok example for Mac and Linux tests and one for Windows. This jinja logic is probably unnecessary if snappy only has a couple libraries.

jakirkham · 2016-04-02T05:28:48Z

recipes/snappy/meta.yaml

+about:
+  home: https://github.com/google/snappy
+  summary: A fast compressor/decompressor
+  license: Other


Looks like BSD 3-Clause. Could we put that here? I know the data is licensed differently, but don't think we want to distribute that anyways.

Note, some of the test data is licensed under different licenses, but since they are not in the resulting binary package, the license stays BSD.

conda-forge-admin · 2016-04-05T22:15:07Z

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/snappy) and found it was in an excellent condition.

jakirkham · 2016-04-06T00:56:45Z

recipes/snappy/meta.yaml

+    - test -e $PREFIX/include/snappy-stubs-public.h  # [unix]
+    - test -e $PREFIX/lib/libsnappy.dylib  # [osx]
+    - test -e $PREFIX/lib/libsnappy.a      # [unix]
+    - test -e $PREFIX/lib/libsnappy.so     # [linux]


Looking good. Could you please indent the selectors in the test section so they are lined up? Also, we may want to test dll and lib for Windows.

Can skip the Windows tests for now, but the other minor comments here should be addressed.

jakirkham · 2016-04-06T01:00:32Z

Thanks for adding tests.

Looks like the Windows builds don't like the version of the solution file in some cases.

The 64-bit Python 2.7 test failure is a bit strange. Not sure what is going on there.

jakirkham · 2016-04-19T02:28:56Z

recipes/snappy/meta.yaml

+
+extra:
+  recipe-maintainers:
+    - groutr


Please add me too. 😄

jakirkham · 2016-04-19T02:32:56Z

Yeah, the one Windows case covered by that solution is passing though. It just doesn't look like we can use it elsewhere.

Looks like they might add CMake support upstream. See this PR ( google/snappy#16 ).

Given that this is building everywhere else and Windows is such a hard target at present. Let's just skip Windows and handle the other quite minor comments here. Once that's done, I would like to get this merged. Sound good everyone?

jakirkham · 2016-04-27T03:12:34Z

Taking this over in this PR ( #459 ) so we can clear this one.

add snappy

fa75e8f

ocefpaf reviewed Mar 23, 2016
View reviewed changes

jakirkham mentioned this pull request Mar 23, 2016

Collections of recipes needing attention? #161

Closed

groutr added 3 commits March 23, 2016 19:05

add make check

c2c9d65

add maintainer

f966259

Bypass autogen.sh

0a08730

include windows build files

8cfcabb

Revert "Bypass autogen.sh"

1833a82

This reverts commit 0a08730.

add libtool and automake dependencies

67d524b

jakirkham reviewed Mar 30, 2016
View reviewed changes

recipes/snappy/bld.bat

REM Build step

devenv "%SLN_FILE%" /Build "%SLN_CFG%|%SLN_PLAT%"

if errorlevel 1 exit 1

Copy link

Member

jakirkham Mar 30, 2016

Choose a reason for hiding this comment

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

Drop extra newline.

jakirkham reviewed Mar 30, 2016
View reviewed changes

groutr added 2 commits March 30, 2016 18:54

add more build dependencies

7823022

state where msvc project files come from

0d2f62d

use copy....not on unix.

abdb9b7

use the correct slashes...not unix again

d8f3b10

use correct variables....

b9df726

jakirkham reviewed Mar 31, 2016
View reviewed changes

jakirkham reviewed Apr 2, 2016
View reviewed changes

groutr added 3 commits April 4, 2016 15:01

add tests for snappy libs

147a46b

make license more accurate for resulting package.

4175c0f

Note, some of the test data is licensed under different licenses, but since they are not in the resulting binary package, the license stays BSD.

use tarball release of snappy

55395fc

jakirkham reviewed Apr 6, 2016
View reviewed changes

jakirkham reviewed Apr 19, 2016
View reviewed changes

recipes/snappy/meta.yaml

extra:

recipe-maintainers:

- groutr

Copy link

Member

jakirkham Apr 19, 2016

Choose a reason for hiding this comment

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

Please add me too. 😄

jakirkham mentioned this pull request Apr 27, 2016

Add snappy (Pt. 2) #459

Merged

ocefpaf closed this in #459 Apr 27, 2016

Add snappy #193

Add snappy #193

Conversation

groutr commented Mar 23, 2016

jakirkham commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groutr commented Mar 23, 2016

jakirkham commented Mar 24, 2016

groutr commented Mar 26, 2016

conda-forge-admin commented Mar 30, 2016

groutr commented Mar 30, 2016

jakirkham commented Mar 30, 2016

conda-forge-admin commented Mar 30, 2016

jakirkham commented Mar 30, 2016

groutr commented Mar 30, 2016

ocefpaf commented Mar 30, 2016

jakirkham commented Mar 30, 2016

groutr commented Mar 30, 2016

conda-forge-admin commented Mar 30, 2016

jakirkham commented Mar 30, 2016

Choose a reason for hiding this comment

jakirkham commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Mar 30, 2016

conda-forge-admin commented Mar 30, 2016

conda-forge-admin commented Mar 31, 2016

conda-forge-admin commented Mar 31, 2016

conda-forge-admin commented Mar 31, 2016

jakirkham commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groutr commented Apr 1, 2016

jakirkham commented Apr 1, 2016

Choose a reason for hiding this comment

conda-forge-admin commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Apr 6, 2016

Choose a reason for hiding this comment

jakirkham commented Apr 19, 2016

jakirkham commented Apr 27, 2016