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

lepton 1.2.1 (new formula) #3012

Closed
wants to merge 1 commit into from
Closed

Conversation

zmwangx
Copy link
Contributor

@zmwangx zmwangx commented Jul 15, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • See explanation below. Does your submission pass brew audit --strict --online <formula> (after doing brew install <formula>)?

This file format and tool is newly open sourced. See
https://blogs.dropbox.com/tech/2016/07/lepton-image-compression-saving-22-losslessly-from-images-at-15mbs/.

It doesn't pass audit at the moment because the GitHub repo is too new, but the code base itself is by no means new — initial commit dates back to June 2015. And Dropbox being Dropbox this repo is quickly gaining traction (500+ stars at time of writing, < 1 day after the blog post).

If the formula isn't acceptable at this point, I'll just leave it around and bump it in a month.

@DomT4 DomT4 added the new formula PR adds a new formula to Homebrew/homebrew-core label Jul 15, 2016
@zmwangx
Copy link
Contributor Author

zmwangx commented Jul 15, 2016

Other than expected audit problem, El Cap:

/usr/local/Library/ENV/4.3/clang++    -I/tmp/lepton-20160715-76959-d9zmzc/lepton-1.0 -I/tmp/lepton-20160715-76959-d9zmzc/lepton-1.0/src/vp8/util -I/tmp/lepton-20160715-76959-d9zmzc/lepton-1.0/src/vp8/model -I/tmp/lepton-20160715-76959-d9zmzc/lepton-1.0/src/vp8/encoder -I/tmp/lepton-20160715-76959-d9zmzc/lepton-1.0/src/vp8/decoder  -std=c++11 -fno-exceptions -fno-rtti -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk -mmacosx-version-min=10.11   -march=core-avx2   -o CMakeFiles/lepton-avx.dir/src/lepton/idct.cc.o -c /tmp/lepton-20160715-76959-d9zmzc/lepton-1.0/src/lepton/idct.cc
/tmp/lepton-20160715-76959-d9zmzc/lepton-1.0/src/lepton/idct.cc:228:15: error: always_inline function '_mm_mullo_epi32' requires target feature 'sse4.1', but would be inlined into function 'idct_sse' that is compiled without support for 'sse4.1'
        xv8 = _mm_mullo_epi32(_mm_set1_epi32(w7), _mm_add_epi32(xv4, xv5));
              ^
        ...

Mavericks and Yosemite:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/7.0.2/include/smmintrin.h:28:2: error: "SSE4.1 instruction set not enabled"

Any insight? I don't see this locally, and this is wandering into the hardware domain which I have absolutely no idea of.

@zmwangx
Copy link
Contributor Author

zmwangx commented Jul 15, 2016

Reported upstream: dropbox/lepton#11.


def install
# Wrong header, fixed in https://github.com/dropbox/lepton/commit/d122450
inreplace "src/vp8/util/memory.cc", "<emmintrin.h>", "<immintrin.h>"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That patch is simply this inreplace plus an irrelevant typo fix in README, so I figured there's no reason to add another download.

Copy link
Member

Choose a reason for hiding this comment

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

Patches are a bit more self-documenting and tend to get removed more readily when not needed so if you could remove it that'd be great.

@zmwangx
Copy link
Contributor Author

zmwangx commented Jul 17, 2016

I've applied a big patch that brings the whole package to latest git master which is dropbox/lepton@acfb6ae. It's not intended for shipping; I just to need to test some upstream fixes and/or improvements on the test bot.

@zmwangx zmwangx force-pushed the lepton-1.0 branch 2 times, most recently from 4a240d0 to e4da5f9 Compare July 17, 2016 23:32
@zmwangx
Copy link
Contributor Author

zmwangx commented Jul 17, 2016

Bumped to 1.2.

@zmwangx zmwangx changed the title lepton 1.0 (new formula) lepton 1.2 (new formula) Jul 17, 2016
@zmwangx
Copy link
Contributor Author

zmwangx commented Jul 18, 2016

We're making good progress here: builds on Mavericks and Yosemite now pass. There's still a (new) problem on El Cap, which I reported upstream in a new issue: dropbox/lepton#28.

@zmwangx zmwangx force-pushed the lepton-1.0 branch 8 times, most recently from ae2560d to 7ac2417 Compare July 18, 2016 05:20
@zmwangx zmwangx changed the title lepton 1.2 (new formula) lepton 1.2.1 (new formula) Jul 18, 2016
@zmwangx
Copy link
Contributor Author

zmwangx commented Jul 18, 2016

@MikeMcQuaid Everything is now sorted out in 1.2.1. Guess we'll just wait for the 30 days mark, or maybe an exception could be made due to popularity (~3000 stars in three days)?

@DomT4 DomT4 closed this in 33601fc Jul 18, 2016
@DomT4
Copy link
Member

DomT4 commented Jul 18, 2016

Yeah, I think the 30 day limit is an inappropriate judge here given the actual codebase is significantly older. We've accepted projects before that are new to GitHub but not "new".

Merged in 33601fc. Thank you once again @zmwangx!

@zmwangx zmwangx deleted the lepton-1.0 branch July 18, 2016 22:44
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants