Skip to content

Migrate from PhantomJS #8

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.

Sign up for GitHub

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

Open
notjosh wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Migrate from PhantomJS #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ccb248a
Bump .gitignore
notjosh Sep 17, 2019
aa98fa4
Switched to headless Chromium from PhantomJS
notjosh Sep 17, 2019
9d74a80
Docker knows how to run Chrome instead of Phantom
notjosh Sep 17, 2019
a2c123e
Matching fonts to match PhantomJS's DejaVu Sans usage
notjosh Sep 17, 2019
636ded7
Let's get buildpacks working with Chrome/headless on Heroku
notjosh Sep 17, 2019
83c8a7c
Maybe app.json replaces the .buildpacks in modern Heroku, let's find …
notjosh Sep 17, 2019
6c79c1b
Oh, that's how buildpacks are named
notjosh Sep 17, 2019
f923357
Turns out chromedriver is on PATH on Heroku already, great!
notjosh Sep 17, 2019
aef831a
We should support emojis by default, so let's take the shortest path …
notjosh Sep 18, 2019
59cc9b0
Merge remote-tracking branch 'origin/master' into feature/no-more-pha…
joerick Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .buildpacks
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# Custom
.vscode/
venv/
.env

Expand Down
31 changes: 22 additions & 9 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,14 @@ FROM python:3.7.4-slim-buster
WORKDIR /sirius

RUN apt-get update -y && \
mkdir -p /usr/share/man/man1 && \
mkdir -p /usr/share/man/man7 && \
apt-get install -y --no-install-recommends \
curl \
gnupg

RUN curl -sS -o - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add \
&& echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list

RUN apt-get update -y && \
apt-get install -y --no-install-recommends \
python3 \
python3-dev \
Expand All @@ -14,20 +20,27 @@ RUN apt-get update -y && \
libjpeg-dev \
libpq-dev \
zlib1g-dev \
bzip2 \
fontconfig \
fonts-dejavu \
fonts-noto-color-emoji \
gcc \
phantomjs \
wget \
google-chrome-stable \
postgresql-11 \
unzip \
git

RUN apt-get autoremove -y

RUN wget https://bitbucket.org/ariya/phantomjs/downloads/phantomjs-2.1.1-linux-x86_64.tar.bz2
RUN tar jxf phantomjs-2.1.1-linux-x86_64.tar.bz2 && \
rm phantomjs-2.1.1-linux-x86_64.tar.bz2 && \
mv phantomjs-2.1.1-linux-x86_64/bin/phantomjs /usr/local/bin/phantomjs
# Find the latest version for `chromedriver` that matches our installed `google-chrome` & install it to be available on $PATH
RUN COMMON_VERSION=$(google-chrome --version | sed -nre "s/.* ([0-9]+\.[0-9]+\.[0-9]+)/\1/p" | cut -d"." -f1-3) && \
URL="https://chromedriver.storage.googleapis.com/LATEST_RELEASE_${COMMON_VERSION}" && \
CHROMEDRIVER_VERSION=$(curl -Ss "${URL}") && \
CHROMEDRIVER_URL="https://chromedriver.storage.googleapis.com/${CHROMEDRIVER_VERSION}/chromedriver_linux64.zip" && \
curl "${CHROMEDRIVER_URL}" -o "${HOME}/chromedriver_linux64.zip" && \
unzip "${HOME}/chromedriver_linux64.zip" -d "${HOME}/" && \
rm "${HOME}/chromedriver_linux64.zip" && \
mv -f "${HOME}/chromedriver" /usr/local/bin/chromedriver && \
chmod 0755 /usr/local/bin/chromedriver

RUN pip install --upgrade pip

Expand Down
37 changes: 37 additions & 0 deletions app.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"name": "Sirius",
"description": "Sirius is a Little Printer server, give or take.",
"website": "https://littleprinter.nordprojects.co/",
"repository": "https://github.com/nordprojects/sirius",
"addons": [
{
"plan": "heroku-postgresql",
"options": {
"version": "11.5"
}
}
],
"image": "heroku/python",
"buildpacks": [
{
"url": "heroku/google-chrome"
},
{
"url": "heroku/chromedriver"
}
],
"env": {
"FLASK_CONFIG": {
"description": "Deployment configuration",
"value": "heroku"
},
"TWITTER_CONSUMER_KEY": {
"description": "Twitter app consumer key",
"required": true
},
"TWITTER_CONSUMER_SECRET": {
"description": "Twitter app consumer secret",
"required": true
}
}
}
19 changes: 13 additions & 6 deletions sirius/coding/default_template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta charset="utf-8" />
<style>
html, body {
html,
body {
margin: 0;
padding: 0;
background-color: white;
Expand All @@ -19,10 +20,15 @@
margin-top: 1px;
margin-bottom: 80px;
border: 1px;
font-family: Helvetica, Arial, sans-serif;
font-family: "DejaVu Sans", Helvetica, Arial, sans-serif;
font-size: 30px;
}
h1, h2, h3, h4, h5, h6 {
h1,
h2,
h3,
h4,
h5,
h6 {
margin-bottom: 20px;
}

Expand All @@ -45,8 +51,9 @@
</head>
<body>
<div class="header">
{{ date.strftime('%H:%M | %d-%b-%Y') }}{% if from_name %} | {{ from_name }}{% endif %}
{{ date.strftime("%H:%M | %d-%b-%Y") }}{% if from_name %} |
{{ from_name }}{% endif %}
</div>
{{ raw_html|safe }}
{{ raw_html | safe }}
</body>
</html>
43 changes: 38 additions & 5 deletions sirius/coding/image_encoding.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,35 @@ def html_to_png(html):
driver = None
try:
caps = {'acceptSslCerts': True}
driver = webdriver.PhantomJS(
'phantomjs', desired_capabilities=caps,
service_args=['--ignore-ssl-errors=true', '--ssl-protocol=any'])
driver.set_window_size(384, 5)
options = webdriver.ChromeOptions()
options.accept_untrusted_certs = True
options.assume_untrusted_cert_issuer = True

options.set_headless()

options.add_argument("--no-sandbox")
options.add_argument("--disable-impl-side-painting")
options.add_argument("--disable-setuid-sandbox")
options.add_argument("--disable-seccomp-filter-sandbox")
options.add_argument("--disable-breakpad")
options.add_argument("--disable-client-side-phishing-detection")
options.add_argument("--disable-cast")
options.add_argument("--disable-cast-streaming-hw-encoding")
options.add_argument("--disable-cloud-import")
options.add_argument("--disable-gpu")
options.add_argument("--disable-popup-blocking")
options.add_argument("--ignore-certificate-errors")
options.add_argument("--disable-session-crashed-bubble")
options.add_argument("--disable-ipv6")
options.add_argument("--allow-http-screen-capture")
options.add_argument('--force-device-scale-factor=1')

driver = webdriver.Chrome(
chrome_options=options,
desired_capabilities=caps,
service_args=['--ignore-ssl-errors=true', '--ssl-protocol=any']
)
driver.set_window_size(384, 8)

# note that the .html suffix is required to make phantomjs
# pick up the mime-type and render correctly.
Expand All @@ -124,7 +149,15 @@ def html_to_png(html):
f.write(html)
f.flush()
driver.get('file://' + f.name)
data = io.BytesIO(driver.get_screenshot_as_png())

total_height = driver.execute_script("return document.body.parentNode.scrollHeight")
driver.set_window_size(384, total_height)
screenshot = driver.get_screenshot_as_png()

# body = driver.find_element_by_tag_name('body')
Copy link
Collaborator Author

@notjosh notjosh Sep 17, 2019

Choose a reason for hiding this comment

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

Don't need to keep this around, but it might be more stable than resizing the viewport? It does break if the height is 0 though, which is kind of undefined right now.

Copy link
Member

@joerick joerick Oct 11, 2019

Choose a reason for hiding this comment

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

Let's leave it commented there, I can imagine that it could come in handy if the window-resizing method has any issues.

Copy link
Member

@joerick joerick Oct 11, 2019

Choose a reason for hiding this comment

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

Just one thought I had - could CSS rules on the html element affect this? I'm guessing that body.parentNode is html... like margin/padding/border on the html element?

Copy link
Collaborator Author

@notjosh notjosh Oct 11, 2019

Choose a reason for hiding this comment

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

Good question! Part of me wonders if we should apply a global CSS reset here to counter against partial/incomplete HTML layouts (say, just <p>hello world</p>) getting padding/margin added, but I can only imagine that going poorly when someone wants a specific layout.

The whole HTML rendering is probably fairly fragile, and it's definitely undefined right now. I'm sure passing something simple in like <meta name="viewport" content="width=device-width, initial-scale=2"> breaks something we care about here.

I was gonna add a bunch of tests that generate PNGs and compare against fixtures, but I'm wondering how fragile that'll be, i.e. different OS fonts will cause test failures. As long as they pass in Docker/on Circle, that's all that really matters I guess? But it'd be good to use it to define a basic spec of "enter this HTML, get this output", and keep it consistent regardless of our implementations here.

Copy link
Member

@joerick joerick Oct 12, 2019

Choose a reason for hiding this comment

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

I was wondering if just a HTML -> image height test would do the trick, although that might fall foul of the fragility problems you raise above re. fonts etc.

HTML passed to the print key API is in fact templated, so <p>Hello, world!</p> would be placed within a HTML document with base styles etc. This is where the Timestamp/from header comes from.

Regardless, we're talking about very edge-casey stuff here, so don't let it hold up your progress!

Copy link
Collaborator Author

@notjosh notjosh Oct 14, 2019

Choose a reason for hiding this comment

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

Good point about the template! I'd forgotten about that.

I added #13 as a starting point to catch any of the main regressions here. The image height test is probably enough to 99% of issues, but having the image snapshots gives us a way to inspect/visualise the differences so I think has a fair bit of value.

So let's get #13 in, then get this PR green against those tests. Then let's merge it in and see how it goes.

# screenshot = body.screenshot_as_png

data = io.BytesIO(screenshot)

return data
finally:
Expand Down
14 changes: 13 additions & 1 deletion sirius/coding/test_image_coding.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,15 @@
from sirius.coding import image_encoding

class ImageCase(unittest.TestCase):
def test_empty_document(self):
data = image_encoding.html_to_png('')
image = Image.open(data)
self.assertEquals(image.size[0], 384)

def test_normal_text(self):
data = image_encoding.html_to_png(
'<html><body style="margin: 0px; height: 10px;"></body></html>'
)
)
image = Image.open(data)
self.assertEquals(image.size[0], 384)
self.assertEquals(image.size[1], 10)
Expand All @@ -20,6 +25,13 @@ def test_normal_height(self):
self.assertEquals(image.size[0], 384)
self.assertEquals(image.size[1], 100)

def test_probably_beyond_viewport_height(self):
Copy link
Collaborator Author

@notjosh notjosh Sep 17, 2019

Choose a reason for hiding this comment

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

Trying to make sure that headless browsers can actually capture a full screenshot, and don't get limited to resizing to fit a virtual screen size (say, 1024x768)

data = image_encoding.html_to_png(
'<html><body style="margin: 0px; height: 10000px;"></body></html>')
image = Image.open(data)
self.assertEquals(image.size[0], 384)
self.assertEquals(image.size[1], 10000)


class PipeTestCase(unittest.TestCase):
def test_specific_image_rle(self):
Expand Down