Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Rebuilding OptiPNG on Unix systems #2

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

badunk commented Feb 14, 2013

This PR rebuilds the OptiPNG project from source on Unix systems if make is installed. It should leave the existing binaries intact otherwise.

I'm not quite sure how the community deals with a missing make command. I had to add XCode binaries it to my path myself on OSX 10.8 (did not ship with make).

lmk if I should refactor some things..switch out directory structures, etc. to comply with coding standards.

@badunk badunk referenced this pull request Feb 14, 2013

Closed

Error thrown on some systems #1

Owner

sindresorhus commented Feb 15, 2013

Worked fine on my OS X 10.7, but I have like everything installed of Xcode, make, etc.

Contributor

badunk commented Feb 15, 2013

Sure those are all great ideas

I've verified on 64 bit linux and it does work. Perhaps you looked at the first commit where I forgot the linux case.

Owner

sindresorhus commented Feb 15, 2013

Perhaps you looked at the first commit where I forgot the linux case.

I meant as precompiled and in the lib/optipng-bin.js file

Contributor

badunk commented Feb 15, 2013

Nice to have for those on 64-bit Linux that can't compile it.

Ah I see what you mean now. Ie, they don't have make. I'll have an update soon

Owner

sindresorhus commented Feb 19, 2013

@badunk sorry for the delay. I'm not notified of new commits. I was thinking. We should probably also have a precompiled binary for OS X 10.7 in addition to the 10.8 that is currently in there. What do you think?

Contributor

badunk commented Feb 19, 2013

While checking in so many binaries permutations seems a bit dirty to me, I think anything to make the user experience in yeoman's toolchain smoother is important.

On the other hand, I didn't realize there's a difference between 10.7 and 10.8. I don't have access to 10.7 sorry

Owner

sindresorhus commented Feb 19, 2013

While checking in so many binaries permutations seems a bit dirty to me, I think anything to make the user experience in yeoman's toolchain smoother is important.

I absolutely agree. But making it easy for the end user it's what's important here.

I have access to 10.7. If you could just mock it for now, and I'll add in that binary later.

Contributor

badunk commented Feb 19, 2013

no problem, I'll have that done tonight

Contributor

badunk commented Feb 20, 2013

@sindresorhus Do you know of any packages that are used to detect that or shall I basically do sw_vers -productVersion

Contributor

badunk commented Feb 20, 2013

If so, I may have to rewrite the export to use deferred/promises/events otherwise..unless there is a spawn/exec sync...

Contributor

badunk commented Feb 22, 2013

Should be good to go, @sindresorhus can you check on your end?

Owner

sindresorhus commented Feb 22, 2013

@badunk landed. Thank you so much for this awesome PR! 🍰

Tried it out on my Mac with OS X 10.8 and was surprised that the 10.7 binary worked there. Looks like you newer version can use older binaries, but not the other way around, which in hinsight makes sense.

I know this is lame, but could you submit a PR with the 10.7/10.8 thing removed and only include the 10.7 binary. Since it isn't needed it would save some space. Actually, maybe we should get a OS X 10.6 binary to reach the widest audience. I'll try to get my hand on one.

I landed this now because it is very much needed in the grunt-contrib-imagemin task. So no time pressure, just nice to have ;)

Would also be awesome to get something similar for the jpegtran task.

Contributor

badunk commented Feb 22, 2013

no problem, will do - thanks!

Owner

sindresorhus commented Feb 22, 2013

@badunk Looks like it's failing on the optipng-rebuild step somehow: https://travis-ci.org/gruntjs/grunt-contrib-imagemin/builds/4989007 Could you take a look?

Owner

sindresorhus commented Feb 22, 2013

nevermind: 8d8707b

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