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

Doesnt work if spawned from another process #68

Closed
milewski opened this issue Aug 10, 2018 · 14 comments
Closed

Doesnt work if spawned from another process #68

milewski opened this issue Aug 10, 2018 · 14 comments

Comments

@milewski
Copy link

milewski commented Aug 10, 2018

When i spawn Crunch from any other process... e.g, node, php, go... the compression ratio is always on 100%

"""
Crunching ...\n
[ 100.00% ] fafdae7fd865c9ea6480e5d5037fd69d-crunch.png (943686 bytes)\n
"""

running the same process manually on bash/sh crunch fafdae7fd865c9ea6480e5d5037fd69d.png gives me

Crunching ...
[ 33.68% ] fafdae7fd865c9ea6480e5d5037fd69d-crunch.png (317815 bytes)

reproducible command

php -r 'shell_exec("crunch path/to/a/png/file.png");'
  • OS: Alpine 3.7
@milewski
Copy link
Author

milewski commented Aug 10, 2018

Well i figured out the reason, im running the process with a different user www-data against root however allowing the user www-data to execute python, crunch, pngquant, zopflipng + full access to the folder it is outputting to... + read on the input file.. doesn't seems to work... the process still outputs [ 100.00% ] no compression, no errors are thrown and process exist with code 0

did i miss something?

@chrissimpkins
Copy link
Owner

Strange. I don’t know what is going on there. Any reason that you can’t call the script with python directly rather than call it through a sub process in a different language? The ‘crunch’ executable is a Python script. If you must execute through a non-Python language it should be fairly simple to re-write the execution directly in one of those languages. There is already a JS implementation if that happens to be helpful to you.

@milewski
Copy link
Author

If I login to my system as the user www-data or any other user really without root access and try to invoke crunch img, python path/to/crunch img I got the same output as the input..

For my use case I have a queue, that spawn crunch instances.. However the queue itself runs as non root account and I can't change that... As far as I know I see that I'm able to execute python and run crunch script with any user... The problem might be on pngquant or zopli not being able to access some system resource under a different account ...

@chrissimpkins
Copy link
Owner

I think that you are correct. The stdout reporting with the % file size calculations are coming from the Python scripting. The Python source is able to read the target file(s) to calculate the file size. For some reason, it appears that optimizations with pngquant and zopflipng are not being performed and the file size remains at 100% original size. Error messages (and non-zero exit status codes) should be propagated from those tools, but that is not happening which is odd.

  1. Let's start with the simplest solution so that we've addressed it. Please confirm that the files have not been previously optimized with pngquant / zopflipng. If they have, clearly there is likely to be no further optimization and the script should appropriately print that the ratio is 100%. I think that this is what you are indicating when you execute the script directly on the command line and see a reduction to ~33% original file size but let's take this off of the table as a possibility.

  2. Can you please confirm that you used the makefile to install the off system PATH versions of pngquant and zopflipng? These are expected on the paths ~/pngquant/pngquant and ~/zopfli/zopflipng, respectively.

  3. Will you please confirm that you tested this with a single file approach? We may be missing error messages/exit status codes if you are batch processing multiple files with the parallel processing approach that is used. Let me know if there is still no error message with single file processing.

  4. Can you confirm that the following also fails execution with the pngquant/zopflipng tools when executed with a working directory that includes the crunch.py source file on the repository path master/src/crunch.py?

php -r 'shell_exec("python crunch.py path/to/a/png/file.png");'

@milewski
Copy link
Author

milewski commented Aug 12, 2018

  1. The image is originally an output of some transformation made by GD, so there is no previously compression being made on this image.

  2. Im building from source using this instructions:

git clone https://github.com/chrissimpkins/Crunch.git --depth 1 && cd Crunch && \
make build-dependencies && make install-executable

if i remove ~/pngquant/pngquant and ~/zopfli/zopflipng i get [ERROR] pngquant executable was not identified on path '/anyone/pngquant/pngquant' so i believe crunch is picking up the correct binaries.

  1. Yes im using one single . png file to run these tests.

  2. the following all fail under a non root account for me:

php -r 'shell_exec("python /usr/local/bin/crunch ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

php -r 'shell_exec("/usr/bin/python /usr/local/bin/crunch ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

// cp  /usr/local/bin/crunch crunch.py
php -r 'shell_exec("./crunch.py 7ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

php -r 'shell_exec("/usr/bin/python ./crunch.py 7ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd.png");'

all had exit code 0 but with [ 100.00% ] compression, switching account back to root all the commands succeed with output:

Crunching ...
[ 22.14% ] 7ZzQou1dwjsn4mGoIbgjKFL6THz8rUnpgl0rWpEd-crunch.png (251325 bytes)

@milewski
Copy link
Author

milewski commented Aug 13, 2018

ok i think i got it figured out now..

when i first install the crunch with this command:

git clone https://github.com/chrissimpkins/Crunch.git --depth 1 && cd Crunch && \
make build-dependencies && make install-executable

im running as root, therefore the ~/pngquant/pngquant and ~/zopfli/zopflipng points out to /home/root/pngquant/pngquant and /home/root/zopfli/zopflipng, when i login with a different user, my home directory becomes /home/another-user/ so i copy the files from /home/root/* to /home/another-user/ then it doesnt work....(running ./pngquant and ./zopflipng manually works)
however if i first install it to a different directory like this:

mkdir /data
git clone https://github.com/chrissimpkins/Crunch.git --depth 1 
cd Crunch
HOME=/data make build-dependencies && make install-executable

crunch will will install the dependencies inside /data/pngquant/pngquant and /data/zopfli/zopflipng, so when i login back as another-user i run crunch like this

HOME=/data crunch img.png
HOME=/data php -r 'shell_exec("crunch img.png");'

and it works :D

@chrissimpkins
Copy link
Owner

Excellent! Do you think that this is a common enough need to warrant placing your approach in the documentation?

@milewski
Copy link
Author

milewski commented Aug 13, 2018

i think the ultimate solution to avoid this problem in the future would be not installing the pngquant and zopflipng to the user ~/ home dir by default.. cuz this is very dependent on who is currently logged in.. + file permissions to access, different users will have different home directories, and as crunch is installed globally it wont be able to access the home folder of other users... perhaps move pngquant/zopflipng to /usr/bin after compilation and expects that they are available globally instead of at a specific path?

@chrissimpkins
Copy link
Owner

perhaps move pngquant/zopflipng to /usr/bin after compilation

The issue with this approach is that it could overwrite a system/package manager/manual install of both project dependencies. This was an intentional decision because:

  • zopflipng is a custom fork of mine that is tailored to crunch PNG file optimization and if /usr/bin is higher priority on $PATH than the package manager/system-installed/manually installed location then the user is unintentionally using this build rather than the upstream default
  • I am intentionally pinning these build dependencies and testing against the pinned versions, cannot test against all rolling versions across all supported OS/Linux distros of these build dependencies to verify that the application works as intended.

@milewski
Copy link
Author

uhm, understood, yeah in that case i think would be worth mentioning on the docs that the installation path can be modified by setting the HOME env to something else, although it was explicitly written $HOME/pngquant/pngquant i didn't realized that was actually a env var until i read the source.

@chrissimpkins
Copy link
Owner

Will update the docs to see if I can clarify this issue. Thanks for helping to get to the bottom of it. I appreciate all of your feedback here!

@chrissimpkins
Copy link
Owner

chrissimpkins commented Aug 14, 2018

Note too that you can specify the path to the pngquant and zopflipng executables in these lines of the script:

Crunch/src/crunch.py

Lines 38 to 39 in e0bca67

PNGQUANT_CLI_PATH = os.path.join(os.path.expanduser("~"), "pngquant", "pngquant")
ZOPFLIPNG_CLI_PATH = os.path.join(os.path.expanduser("~"), "zopfli", "zopflipng")

It is not necessary to use os.path.join if you are using this on a known single platform, just replace the entire os.path.join(...) statement with a string that is the filepath to these executables.

PNGQUANT_CLI_PATH = "path/to/file" 
ZOPFLIPNG_CLI_PATH = "path/to/file"

The Python os.path.join code there makes the paths platform independent. This should eliminate the need for you to set environment variables. Sorry for the confusion!

@milewski
Copy link
Author

uhm but that would require having a fork of this repo which potently could lead me for missing further updates or writing a script to find/replace on the crunch.py, currently for my use case im building it inside a docker container.. could that PNGQUANT_CLI_PATH and ZOPFLIPNG_CLI_PATH be taken from ENV something like

PNGQUANT_CLI_PATH = env.PNGQUANT_CLI_PATH || os.path.join(os.path.expanduser("~"), "pngquant", "pngquant") (sorry i dont know python)

chrissimpkins added a commit that referenced this issue Aug 14, 2018
adds documentation for install account vs. execution account issue that was raised in #68
@chrissimpkins
Copy link
Owner

Added documentation in the Install section of the docs/EXECUTABLE.md document in ee00d14

I am definitely open to considering environment variable support for installations and execution. Mind opening a new issue report with details on how the installation and execution should be modified to support your use case (or a PR with the proposed changes)?

Thanks again for your help here!

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

2 participants