| | <<<Back 1 day (to 2016/09/12) | 20160913 |
| Robin_Watts | I get no reference counting issues when using the gproof device on the desktop. | 10:46.48 |
| | oops, wrong group | 10:47.10 |
| sebras | tor8: trivial patch over at sebras/master (actually yours), can I merge it? | 14:00.13 |
| | Robin_Watts: given your previous work with having cygin on windows work with the cluster. do you think it is feasible to have android also build automatically? | 14:01.17 |
| Robin_Watts | sebras: I believe it's doable. It would require a change to the cluster though. | 14:01.43 |
| sebras | realizes his grammar now is messed up in any language. | 14:01.45 |
| Robin_Watts | IMHO, cluster nodes should announce their capabilities when connecting to the clustermaster. | 14:02.19 |
| sebras | Robin_Watts: if the cost is not huge we would be able to avoid those silly ooops forgot a build system I'm not currently using mistakes. | 14:02.28 |
| Robin_Watts | and then the clustermaster can feed them particular jobs. | 14:02.39 |
| | The downside to that plan would be that we'd have to cope if there wasn't (say) a windows build box available. | 14:03.22 |
| sebras | Robin_Watts: right. is that an implicit failure? | 14:04.31 |
| | would definitely put more strain on whomever is responsible for keeping the cluster up. | 14:04.49 |
| Robin_Watts | Either it would be an implicit failure, or we'd have to keep the job queued until we did have one. | 14:05.04 |
| sebras | Robin_Watts: I think failure is better. because whomever fired up the job would go looking at the log, but if no response is sent..? | 14:05.56 |
| Robin_Watts | I think I agree. | 14:06.08 |
| | actually, the way it'd probably work is that if you schedule a ghostscript cluster job, it would sent "build+test" to all the connected linux nodes, and just "build" to all the connected windows nodes. | 14:07.11 |
| | hence not having a windows node connected would mean that you'd not get an error. | 14:07.34 |
| sebras | Robin_Watts: ok. and if you _do_ have a windows node and the build fails..? | 14:08.17 |
| Robin_Watts | Then that would be an error. | 14:08.25 |
| sebras | that would still be visisble as an error? | 14:08.26 |
| | ok, that would probably work for me. | 14:08.33 |
| | and if it can be coaxed into working for android, even better. | 14:08.44 |
| | do we build for ios as it is? | 14:08.51 |
| Robin_Watts | sebras: We do not. | 14:09.07 |
| sebras | Robin_Watts: but there is a macpro? but it's never scheduled? | 14:09.40 |
| Robin_Watts | That would be possible too, but it would have to require someone else being prepared to setup and maintain the ios build tools etc | 14:09.54 |
| | Cos I am not going near that. | 14:10.09 |
| sebras | Robin_Watts: :) I'd place my bet on jogux. | 14:10.36 |
| HenryStiles | I think for mobile we should be looking at other test sites and not roll our own. | 14:11.32 |
| Robin_Watts | HenryStiles: This would just be smoking testing builds. | 14:12.03 |
| | s/smoking/smoke/ | 14:12.11 |
| sebras | Robin_Watts: smoke tests imply _some_ kind of test at all right, we're only talking about build testing here, really. | 14:12.42 |
| Robin_Watts | ok, "build testing" then. | 14:12.59 |
| HenryStiles | but build testing and running on many devices? | 14:13.18 |
| Robin_Watts | HenryStiles: No. | 14:13.36 |
| sebras | HenryStiles: that wasn't my intention. | 14:13.39 |
| | HenryStiles: I recently added files to mupdf that broke the windows build (because I normally don't build it) | 14:14.03 |
| HenryStiles | just the build, doesn't seem like it would be worth the trouble but I'm fine with it. | 14:14.16 |
| Robin_Watts | All that sebras and I were discussing was attempting to leverage to cluster so as to prevent us getting into the situation where we commit a change and forgetting to update all the relevant build things. | 14:14.29 |
| sebras | HenryStiles: if it is easy enough to at least build for win in the cluster then that would resolve my mistakes for messing things up for robin. | 14:14.42 |
| HenryStiles | fair enough | 14:15.01 |
| sebras | HenryStiles: Robin_Watts: if you guys deem it not worth the effort I'm fine with that too. but I'll probably repeat my mistake again (and no one calls me out on it in the review either! ;) ) | 14:15.48 |
| HenryStiles | the macpro lives with me, if somebody wants to send a batch process that builds mupdf ios (if that is even possible) I'll add it. | 14:16.36 |
| Robin_Watts | sebras: We've all done it before. We'll all do it again. It's just unfortunate that this time it hit michael at an inconvient point. | 14:16.44 |
| | HenryStiles: You've not had the pleasure of building for ios have you? :) | 14:17.08 |
| HenryStiles | Robin_Watts: I have built the ios app, I have no idea how to do it outside of xcode from the command line. | 14:17.41 |
| Robin_Watts | Many strange incantations, installations of xcodes, sacrifices of chickens, obscure provisioning keys, burning candles and a sprinkling of luck are required. | 14:18.17 |
| tor8 | sebras: LGTM. | 14:18.32 |
| Robin_Watts | And you have to redo them about as often as you have to reboot a Windows ME box. | 14:18.35 |
| | I'm a fan, you can tell. | 14:18.56 |
| HenryStiles | jogux is the shaman | 14:19.29 |
| tor8 | Robin_Watts: Shhh. Don't tell anyone the real reason I stopped using Apple software... | 14:19.31 |
| jogux | it's not too bad setting up iOS. Apple made it a lot easier in Xcode 8 which is officially released in 2 hours time :-) | 14:20.52 |
| | command line builds are easy enough, you just call xcodebuild or use fastlane. latter is meant to be very good, but seems to introduce as many problems as it solves so I'm veering back to the former lately. | 14:21.32 |
| Robin_Watts | tor8 uncloaks! | 14:21.45 |
| sebras | HenryStiles: would our app(s) build easily in those thirdparty test environments? | 14:21.55 |
| Robin_Watts | tor8: To return to the font bbox commit on robin/master... | 14:22.26 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=005dbf1594bf24011b633df6d96685883fb59a1d | 14:22.34 |
| HenryStiles | sebras: I haven't looked much at it. I think jogux looked into a few services | 14:22.55 |
| Robin_Watts | You previously complained because you said that font->bbox_table was not always defined. | 14:22.57 |
| | sebras: I don't think we care about the current ios app, cos it's not in the ideal form. | 14:23.18 |
| | We care much more about the new one. | 14:23.24 |
| | tor8: In every case where we call either of the functions that rely on it, we know it exists (because we've just checked for it). | 14:23.52 |
| | Hence I reckon the commit is good to go. | 14:24.00 |
| sebras | tor8: commits LGTY and clusters, so I'll push it. | 14:24.09 |
| jogux | sebras: Yeah, easy enough to get mupdf building in third party environments (more so if it builds nicely in android studio now). | 14:24.10 |
| Robin_Watts | It passes the cluster, with 1 progression (the testfile for the bug in question) | 14:24.15 |
| jogux | sebras: I think the ones Henry is talking about are things like Amazon's cloud testing, where we actually check that the android build *runs* on x billion Android devices. | 14:24.56 |
| HenryStiles | if it isn't much more work to have testing beyone build I think that would be really great | 14:25.17 |
| sebras | jogux: right, but then we'd need to define how to test it as well? | 14:25.21 |
| jogux | sebras: Yeah. Although "app starting and can load a document" is a damn good start :) | 14:25.49 |
| sebras | jogux: i.e. what files to use and where the automated virtual finger should be clicking and what strings should be entered through the keyboard etc... | 14:26.00 |
| tor8 | Robin_Watts: right you are. LGTM then. | 14:26.02 |
| Robin_Watts | tor8: Thanks./ | 14:26.12 |
| tor8 | Robin_Watts: all type 3 fonts will have <= 256 glyphs, so should always have a bbox_table so we won't run into the issue of having to fall back to the font->bbox | 14:27.13 |
| Robin_Watts | tor8: yeah. | 14:27.48 |
| tor8 | which is also highly unreliable, but tends to work okay enough for CJK fonts which is where we tend to end up using it most | 14:28.15 |
| sebras | tor8: with my commit now on master you should not need 86864c8 any more. | 14:46.45 |
| tor8 | sebras: yes, git rebase automatically fixes that (and drops the commit) | 14:50.35 |
| sebras | tor8: cool. | 14:51.34 |
| | tor8: will you make the pdf-annot.c pdf-annot-edit.c merger commit today? | 14:51.54 |
| | and i' | 14:51.56 |
| | ll rebase my changes and remove the cruft tomorrow? | 14:52.05 |
| tor8 | sebras: I think I'll keep the split (just move a few functions from pdf-annot to pdf-annot-edit/pdf-appearance) | 14:59.18 |
| | so that pdf-annot.c will have the read-only basic functions for the fz_annot layer | 14:59.35 |
| | pdf-annot-edit.c will have all the creation/deletion and property accessors | 14:59.50 |
| | pdf-appearance.c will have all the appearance synthesis | 14:59.59 |
| | the stuff on tor/master could do with a review | 15:00.11 |
| | (and a thorough test, I am afraid that the tmp/deleted/changed annotation lists commit will break some of the viewers that I'm not in a position to test right now | 15:00.40 |
| sebras | tor8: aren't those just a matter of search/replace? | 15:01.09 |
| tor8 | sebras: search/replace what? | 15:02.03 |
| sebras | FZ_ANNOT_FREETEXT -> PDF_ANNOT_FREE_TEXT | 15:02.19 |
| | or it is not that one you're refering to? | 15:02.31 |
| tor8 | no. I'm referring to ad21b48 | 15:02.56 |
| sebras | aha. | 15:03.22 |
| | tor8: both the old android viewer and ios apperas to call pdf_poll_changed_annot() which you seem to remove. | 15:06.44 |
| | platform/android/viewer/jni/mupdf.c::update_changed_rects() calls it. | 15:07.26 |
| | same with platform/ios/Classes/MuPageViewNormal.m::updatePage() | 15:07.49 |
| tor8 | sebras: yeah. I changed how it's used in pdfapp.c | 15:08.36 |
| | just normal iteration with an if (annot->changed) test for the block | 15:08.56 |
| | that pattern should be used for the other viewers too | 15:09.06 |
| sebras | tor8: conceivably introduced in the same commit! ;) | 15:09.38 |
| | I attempted to see if the apps use any of the other pdf_page mambers that you remove, but I don't see any of that. | 15:10.14 |
| | that's what I can find. | 15:11.43 |
| tor8 | sebras: tip of tor/master | 15:14.08 |
| | sebras: well, I guess I can fix the android viewer | 15:15.03 |
| | the iOS viewer will be more difficult | 15:15.11 |
| sebras | tor8: let me relieve you of that clang warnings in mupdf_native.c | 15:16.41 |
| | I don't see those using gcc... | 15:16.49 |
| tor8 | if you can teach gcc about setjmp so we don't get the dozens of 'possibly uninitialized value' warnings that'd be swell | 15:18.07 |
| sebras | tor8: where do you get those? | 15:18.36 |
| tor8 | sebras: I already fixed 52394dba7f4714ec22bb351068e86303706a0238 | 15:19.02 |
| sebras | tor8: what about the warnings for initializing fz_rect using { 0 } ? | 15:19.27 |
| tor8 | sebras: everywhere, when building release with gcc | 15:19.39 |
| sebras | clang 3.6.2 warns on those. | 15:19.43 |
| tor8 | sebras: we don't need to initialize the hits[] array | 15:20.53 |
| sebras | I know. I'd like to merge your len= 0 change with not initing those hits arrays. | 15:21.17 |
| tor8 | nor the pdf_write_options (pdf_parse_write_options fully initializes it) | 15:21.31 |
| sebras | also I'd prefer to return 0 instead of initing length to zero (to keep the code consistent) | 15:21.33 |
| | I know. | 15:21.40 |
| tor8 | initing len to zero is consistent with how I handled jni_rethrows everywhere else | 15:22.05 |
| | maybe you've changed how that works though | 15:22.14 |
| sebras | I think I have. if the function returns I try to return the fixed value instead so it stands out and you don't need to go hunting for it and see if was accidentally modified on the path to the return. | 15:23.06 |
| tor8 | sebras: in Buffer_readByte, shouldn't that if (at >= buf->len) also throw an exception? | 15:23.23 |
| | if (jat < 0 || jat >= buf->len) throw_oob | 15:24.18 |
| sebras | possibly, let me check InputStream interface | 15:24.30 |
| tor8 | likewise for readBytes | 15:24.38 |
| | etc | 15:24.46 |
| sebras | tor8: actually InputStream.read() returns -1 on EOF | 15:25.26 |
| | it's quite ugly though because in BufferInputStream I just continue incrementing position and relying on that Buffer.readByte() returns -1 | 15:26.07 |
| | the same goes for InputStream attempting to read into byte[] | 15:27.36 |
| | -1 is returned upon EOF. | 15:27.42 |
| | tor8: oh, that's were there NPE were thrown... if the byte array is null. | 15:36.16 |
| | the jdk interface stipulates that. | 15:36.23 |
| | I'm going to say we stay with IAE in Buffer() and I make a test in the java code. is that ok? | 15:36.46 |
| | if (b == null) throw NPE() | 15:36.54 |
| | that way we still conform to InputStream interface while also making all mupdf interfaces behave consistently. | 15:37.26 |
| tor8 | you mean have duplicate tests for the same thing in Buffer.java and mupdf_native.c? | 15:38.39 |
| sebras | mmm | 15:39.04 |
| | a bit icky. | 15:39.11 |
| tor8 | the second test in mupdf_native.c would be completely redundant | 15:39.26 |
| sebras | no, only if you use BufferInputStream. | 15:39.42 |
| | if you work on the Buffer directly we could throw there. | 15:39.54 |
| tor8 | right now we're going to throw IAE and OOB exceptions instead of IO exceptions in BufferInputStream | 15:43.15 |
| | try/catch/throw IOException in the BufferInputStream java methods? | 15:43.37 |
| | or is the code well behaved and never provokes an OOB? | 15:44.10 |
| | sebras: anyway, time to eat. TTYL. | 15:45.39 |
| sebras | tor8: the idea was that it should be well behaved. | 16:23.28 |
| Robin_Watts | tor8, sebras, mvrhel_laptop (or fred when he gets back): I have 3 commits on robin/master for mupdf to be reviewed. | 16:40.08 |
| | numbers 3/4/5 in the list. | 16:40.22 |
| | 5 updates memento to keep backtraces on android. | 16:41.03 |
| | 4 updates the android viewers build so it works with that. | 16:41.14 |
| | 3 fixes a bug with the GPRF device building. | 16:41.27 |
| mvrhel_laptop | Robin_Watts: so the one the gprf device. Is that this one? http://git.ghostscript.com/?p=user/robin/mupdf.git;a=blobdiff;f=source/gprf/gprf-doc.c;h=a4658d228a37f7ea6509796a9d1cb2f835dc1c96;hp=6308dc8f1585d26f9c814e78b01f3bab41c0bfe1;hb=2c907c5e034b140d812202b63bb533d23d70445c;hpb=c4cccf356eb9ec2300a7f3250d5b95a788e9213e | 16:43.36 |
| Robin_Watts | Yes. | 16:43.45 |
| | Pants. | 16:43.48 |
| | Just a mo, sorry. | 16:43.55 |
| | Fixed version online, sorry. | 16:45.09 |
| mvrhel_laptop | ok. much better :) | 16:45.36 |
| | That one lgtm | 16:45.52 |
| | I think you will need fred for the others.... | 16:46.20 |
| Robin_Watts | mvrhel_laptop: Possibly. I'm not expecting anyone else to understand the internals of memento, but fred will probably recognise the code from a stackoverflow page we've both been looking at :) | 16:47.37 |
| sebras | Robin_Watts: why is status in the outerscope in Memento_showStacktrace()? | 16:56.37 |
| Robin_Watts | sebras: no particular reason. | 16:59.24 |
| | I hacked the code around a lot, and that's just where it ended up. | 16:59.34 |
| | Would have been neater inside, but too late now. | 16:59.40 |
| sebras | https://gcc.gnu.org/onlinedocs/libstdc++/libstdc++-html-USERS-4.3/a01696.html | 17:01.10 |
| | output_buffer can be reallocated using realloc(). | 17:01.25 |
| Robin_Watts | sebras: Oh, pants. | 17:01.50 |
| sebras | but if you can vouch for 256 bytes always being enough I don't have a comment. :) | 17:01.52 |
| Robin_Watts | I'll fix that, thanks. Ignorance on my part. | 17:02.08 |
| sebras | also the pointer demangled must be free()d. | 17:02.35 |
| | according to the link. presumably because malloc() was used to allocate it. :) | 17:03.00 |
| | could the two memento patches be one commit? | 17:04.17 |
| Robin_Watts | sebras: Yeah, I was hoping to avoid mallocing/freeing. | 17:04.24 |
| | sebras: No. One is a change to memento, one is a change to code that uses memento. One I need to pull over into gs :) | 17:04.44 |
| sebras | Robin_Watts: ok, if you want to keep the history of the two files in synch I see your point. | 17:06.11 |
| | Robin_Watts: add missing inclue looks good to me. | 17:07.49 |
| | Robin_Watts: the svg device change I think I have reviewed before. | 17:08.28 |
| Robin_Watts | sebras: Thanks, mvrhel reviewed that in the other tab already, as did fred for the memento things, but he missed your insight into the reallocing. | 17:08.32 |
| sebras | ok. | 17:08.44 |
| Robin_Watts | I'll get a followup commit up with your fix in. | 17:08.52 |
| sebras | I was just curious enough to actually read the manpage of the functions you call. :) | 17:08.59 |
| Robin_Watts | Not even I did that :) | 17:09.10 |
| sebras | I didn't read the other tab, sorry. :) | 17:09.14 |
| | when I review code I always do. for all functions, mainly for checking boundary conditions. | 17:09.36 |
| | I often find something. | 17:09.40 |
| Robin_Watts | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=a1a5fc2d220c66f316e4c527de4ae6719c6fba35 | 17:10.07 |
| sebras | Robin_Watts: LGTM, but I don't know if it works now so please test it. :) | 17:11.25 |
| Robin_Watts | just doing that. | 17:11.34 |
| | Forward 1 day (to 2016/09/14)>>> | |
IRC Logs