Update deps

[CraftSpider]

Edit: Updates watchexec and hyper to the latest versions, and makes code and test changes necessary for them to compile, as well as adding a new test for -X watch to hopefully catch any future regressions in basic functionality.

[CraftSpider]

This got moved into the thread because tectonic uses the reqwest blocking client, which is incompatible with an outer runtime executing at the same time, but also needs a client active at the time of call to Server::bind. The fundamental issue is that we're mixing an async library (hyper) with a non-async one (tectonic) that is using an async one (reqwest). A 'real' fix would be to make tectonic async, but that seems... difficult, and possibly undesirable.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@ec275c6). Click here to learn what that means.
The diff coverage is 3.44%.

❗ Current head fe79948 differs from pull request most recent head c7eb6a7. Consider uploading reports for the commit c7eb6a7 to get more accurate results

@@            Coverage Diff            @@
##             master    #1033   +/-   ##
=========================================
  Coverage          ?   44.84%           
=========================================
  Files             ?      155           
  Lines             ?    62605           
  Branches          ?        0           
=========================================
  Hits              ?    28074           
  Misses            ?    34531           
  Partials          ?        0           

Impacted Files	Coverage Δ
src/bin/tectonic/v2cli.rs	42.26% <0.00%> (ø)
src/bin/tectonic/watch.rs	0.00% <ø> (ø)
crates/engine_bibtex/bibtex/bibtex.c	44.75% <33.33%> (ø)
src/driver.rs	75.65% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[pkgw]

Thank you so much for tackling this!

I will review everything a little later, but it looks like rustfmt has a few complaints that should be easy to fix if you have a chance to do so. If not, I can push a commit myself.

[CraftSpider]

I can fix rustfmt later tonight, though I don't mind if you want to do it now.

[pkgw]

Thanks again for working on this! I have a couple of small stylistic notes, and one main thing where I get a crash when I run a watch command. Do you see the same thing?

[CraftSpider]

Fixed watch execution and added a simple integration test for it - the test passes locally now, I'll fix it if CI complains.

[CraftSpider]

Hmm, the failure might be that cross-testing is just slow enough it doesn't have time to work, or might be an actual failure of watch to do anything. I'll have to see if I can make the test not just use a flat timer, but some other solution.

[CraftSpider]

@pkgw I'm stuck on this - see #1035 for my issue, basically, I'm not sure how to debug the non-rustfmt failures, I'd appreciate some help if you can spare any.

[CraftSpider]

Update: I got the tests working locally, and as far as I can tell, the issue is simply that the emulator is slow and it's hard to actually tell when it starts watching. For now I just pushed the timeouts up to hopefully sufficient numbers. If this still doesn't work I'll come back to it after #1038 is merged, if it does work I'll close that in favor of just merging this.

[pkgw]

Thanks again for plugging away on this! Looks like the timing is still an issue, unfortunately. I am OK with skipping the relevant test on the cross platforms in the name of pragmatism.

Also, you'll likely have seen that I just merged #1038.

[CraftSpider]

Rebased. I'm okay with skipping the test if you are, I'm mostly just kind of annoyed that I can't find a great solution because technically the native tests could fail, even though they haven't so far, and I'm hesitant to introduce a possibly-flaky test.

One possible solution is maybe to just try modifying the file every couple seconds until it detects the output having rebuilt at least twice - that should work even on the slow test runners, as last run they did manage to get in one rebuild before getting deadlocked, so just not waiting on them might be fine, and avoid deadlock by removing the dependence. It's slightly less precise of a test, but it still shows watch working, so I think it would be fine as a basic smoke test.

[pkgw]

Yeah, this kind of thing is definitely frustrating, but these cross-build environments are just inherently inconvenient to work with sometimes. And, I'm confident in the belief that it's very, very unlikely that this particular test would reveal a genuine problem that we wouldn't discover some other way.

[CraftSpider]

That's fair. I cfg-gated the test off mips for now - even running unconditionally, it now succeeds cross locally but fails on the test runner with a bunch of 'FPU mode' errors, which makes me think the emulation may be a bit messed up somewhere. Either way, I agree that the test isn't super likely to catch something on mips we wouldn't notice otherwise, the test as a whole I only really wanted to add so CI wouldn't pass anything obviously wrong.

[pkgw]

D'oh, one last thing to deal with (hopefully) — it looks like the coverage runs have been timing out pretty consistently as well, with the v2_watch_succeeds test hanging. Normally I'd volunteer to try to track the issue down myself but I have some pressing work deadlines, so unfortunately I don't expect to have the chance to do so within the next few days.

The coverage builds are a little finicky to set up — if you want to try to generate one, I'd recommend checking out the CI scripts to see how they do it. When running in coverage mode in the CI system, I recall that Tectonic needs to re-execute itself with a special wrapper, so I could imagine various ways that might go wrong with the watch functionality.

[CraftSpider]

Thanks for figuring it out, sorry I didn't get to it first.

[pkgw]

Thanks for all of your work on this! I had an unexpected opening in my schedule so it was nice to be able to sit down and pitch in a little bit.