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

Make deploy (and make remote) fail when EXTRA_FILES exist #368

Open
miketaylr opened this issue Jun 2, 2021 · 8 comments
Open

Make deploy (and make remote) fail when EXTRA_FILES exist #368

miketaylr opened this issue Jun 2, 2021 · 8 comments

Comments

@miketaylr
Copy link
Member

STR:

  1. clone the compat spec
  2. run make remote
❯ make remote
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 31897    0   249  100 31648    136  17369  0:00:01  0:00:01 --:--:-- 17496

Error running preprocessor, returned code: 2.
WARNING: Couldn't determine width and height of this image: stroked-text.png
[Errno 2] No such file or directory: '/tmp/stroked-text.png'
 ✘  Did not generate, due to fatal errors
make: *** [remote] Error 22

I guess the Bikeshed API gets sad because it can't find the stroked-text.png image, and then bails.

Looking at https://tabatkins.github.io/bikeshed/#curl, if I change -F die-on=warning to F die-on=fatal, make remote doesn't fail, but I'm not sure if that's what we want.

(Another idea might be to upload static assets to something like resources.whatwg.org, and rewrite relative references. But that sounds like more work?)

@sideshowbarker
Copy link
Contributor

So per https://matrixlogs.bakkot.com/WHATWG/2021-06-02#L81

i just found that if i change -F die-on=warning to F die-on=fatal, it seems to work as expected

…you mean that causes the image to get used as expected?

That seems to suggest the WARNING: Couldn't determine width and height of this image: stroked-text.png problem is what causes the failure when die-on=warning is set. And that warning is coming from Bikeshed, so that seems to indicate the image at least got successfully copied to where Bikeshed expects to find it… maybe?

So sounds like some insight from @tabatkins on this might be needed.

@miketaylr
Copy link
Member Author

…you mean that causes the image to get used as expected?

Actually... no. So that's obviously not a great path forward.

@miketaylr
Copy link
Member Author

https://tabatkins.github.io/bikeshed/#img-size

Note: Bikeshed will only successfully detect the size of local images, and will not attempt to fetch resources over the network, as there is no way for Bikeshed to know whether the size it would get is stable. Using local images is generally preferred, but if you need to use a remote one, consider setting the width and height manually.

I guess the issue here is the fact that bikeshed doesn't know about these EXTRA_FILES, because they're not actually sent. So it bails.

curlbikeshed() {
# The Accept: header ensures we get the error output even when warnings are produced, per
# https://github.com/whatwg/whatwg.org/issues/227#issuecomment-419969339.
HTTP_STATUS=$(curlretry https://api.csswg.org/bikeshed/ \
--output "$1" \
--write-out "%{http_code}" \
--header "Accept: text/plain, text/html" \
-F die-on=warning \
-F file=@"$INPUT_FILE" \
"${@:2}")
if [[ "$HTTP_STATUS" != "200" ]]; then
cat "$1"
rm -f "$1"
exit 22
fi
}

@domenic
Copy link
Member

domenic commented Jun 2, 2021 via email

@miketaylr
Copy link
Member Author

It seems like you could fix this by adding width="" and height="" attributes to the source, maybe?

Yeah -- I was just filing an issue to do that as a workaround for the compat spec.

@tabatkins
Copy link
Contributor

Yeah, it's good practice to add width/height anyway, so Bikeshed started warning when it couldn't auto-determine them for you.

@tabatkins
Copy link
Contributor

so that seems to indicate the image at least got successfully copied to where Bikeshed expects to find it… maybe?

Nah, the error message makes it clearer that it's not finding the image at all:

WARNING: Couldn't determine width and height of this image: stroked-text.png
[Errno 2] No such file or directory: '/tmp/stroked-text.png'

@annevk
Copy link
Member

annevk commented Sep 7, 2021

@plinss @tabatkins shouldn't the API-based Bikeshed not try to open such an image or are we expected to include the images in the payload somehow? (If so, how?)

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

No branches or pull requests

5 participants