Code review comment for lp:~michihenning/unity-scopes-api/abigail

Revision history for this message
Michi Henning (michihenning) wrote :

> This looks pretty good. I've left a few inline comments about possible
> simplifications/improvements, but this is definitely on the right track.

Thanks for the review!

I've fixed the gunzip issue and added back a comment in HACKING about where to find the report

With the cmake thing, I stuffed around with that for quite a while. The custom command to dump the abi is necessary because it needs to be dumped whenever it isn't there yet, or after the library has been rebuilt.

For the check-abi step, I want to make sure that the test runs every time, whether the abi dump is new or not. With a naive approach, using add_custom_target/add_custom_command, the problem was that, after the test was run once, running it again wouldn't do anything, even when the ABI was broken (because the report file was up-to-date).

But, to force re-generation of the abi dump if necessary, I *have* to use a a custom command that depends on the dump file. In the end, the only reliable way I managed to come up with was to use the dummy output file. If there is a simpler way of doing this, I'd like to find out though. Every time I use add_custom_target/add_custom_command, my mind turns into a pretzel. I find this uniquely awkward to use, much worse than a makefile, as a matter of fact :-(

With the suppressions, things are in progress. There *is* a bug, but in a slightly different shape than I thought. Dodji has come up with a work-around the deals with the problem I was seeing though. So, on Monday, I think I'll be able to turn suppressions on and do things properly (with a slight hack in the suppressions file). I'll push another branch then with a link to the bug tracker as appropriate.

If there is any cargo-culting, it'll at least be cleaner cargo after that :-)

PS: Have you ever read "Dream Park--The California Voodoo Game"? If not, do. You will probably enjoy it :-)

« Back to merge proposal