Skip to content

Add homepage/categories/keywords to package metadata. #42

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

Jump to bottom
Merged
merged 2 commits into from
Dec 6, 2020
Merged

Add homepage/categories/keywords to package metadata. #42

merged 2 commits into from
Dec 6, 2020

Conversation

mkantor
Copy link
Owner

@mkantor mkantor commented Dec 5, 2020

No description provided.

@mkantor
Copy link
Owner Author

mkantor commented Dec 5, 2020 •
edited
Loading

Hm, not sure what's up with this CI failure:

Rendering /favicon as image/x-icon produced different results when done via server and get command!
    get command exit code: exit code: 0
    get command stdout: 14778 bytes
    get command stderr: b""
    HTTP status code: 200 OK
    HTTP response body: 15406 bytes

I can't reproduce this locally, and haven't observed this before in CI. It does appear to be reliably reproducible in the CI job, but it seems very implausible that this is related to the changes from this branch. Given all that, my guess is that something changed in the environment to break things. Time to do some digging...

Additional findings so far:

  • This test fails the same way in both the macos and ubuntu jobs.
  • It fails in the current master branch too.
  • The only file that fails is favicon.ico from the realistic-basic sample.
  • It fails when that favicon.ico file is requested independently of any others (so it's not related to server load or anything).
  • The actual difference in rendered bytes is that the HTTP response body has 628 null bytes at the end which are not present in the get output. Besides that they're identical.
  • When I run the tests locally, both get output and the HTTP response have those trailing null bytes (they're both identical to the HTTP response in CI).
  • The actual favicon.ico file sitting on my filesystem is 15406 bytes and includes those trailing null bytes.
  • Sanity-checking the favicon.ico file at rest in the CI environment shows that it's definitely 15406 bytes on both macos and ubuntu.
  • This is reproducible without any of my code on the receiving end. Just piping out the bytes from operator get ... also produces a result that is missing bytes at the end of the stream.

Takeaways:

  • Somehow when this favicon.ico file is being read from Rust (and then only in the operator get ... code path), 628 trailing null bytes are being dropped (or maybe they're correctly read but being dropped on their way to stdout).

UPDATE: I finally figured this out, and the actual problem was just a somewhat-embarassing misunderstanding of the io::Write methods on my part. More details plus a fix are in #43. I'm still not sure exactly what changed in the GitHub Actions runners to start triggering this bug (the infrastructure is intentionally opaque), but it was definitely a legit bug in my code, just lying in wait until now.

@mkantor
Copy link
Owner Author

mkantor commented Dec 6, 2020

To make it abundantly clear, by now I know for sure that this CI failure has absolutely nothing to do with this changeset. However I've already started taking notes here and I'm in no rush to merge this, so I'll keep this umerged for now and continue to update the previous comment.

mkantor added a commit that referenced this pull request Dec 6, 2020
@mkantor
Bugfix: get and eval subcommands could sometimes omit bytes.
a433f8a 
Bytes from the end of content streams could be missing from Operator's
output.

I'd somehow gotten it into my head that in `io::Write`, `write` +
`flush` was the same as `write_all` (i.e. that non-written bytes were
held in some internal buffer that got emptied upon `flush`). In
hindsight this is silly, since `io::Write` is a trait there's not even
anywhere to store such a buffer (individual `Write` impls could work
this way in theory, but at least the one for `io::StdoutLock` doesn't).

I hadn't noticed this problem until now because apparently `write` had
been writing the entire buffer at once. Something must have changed in
the GitHub Actions runners, though, as the issue was 100% reproducible
for a certain sample in <#42>
and all CI runs since that PR was opened.
@mkantor mkantor mentioned this pull request Dec 6, 2020
Merged
@mkantor mkantor force-pushed the flesh-out-package-manifest branch from d536c93 to e1045d5 Compare December 6, 2020 21:46
@mkantor
Copy link
Owner Author

mkantor commented Dec 6, 2020

I've rebased this atop master now that #43 has been merged. CI should go green now (🤞).

@mkantor mkantor merged commit b029f83 into master Dec 6, 2020
@mkantor mkantor deleted the flesh-out-package-manifest branch December 6, 2020 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@mkantor