16690 breaks `make check` [WAS Re: AUDIT: r16690 - gnucash/trunk/src/engine - Daniel Harding's update to Afghani currency. closes #504257]

[Daniel Harding]

[resending to the list, because I realized my original reply went only 
to Andrew]

Andrew Sackville-West wrote on 24 Dec 2007 9:12 PM:
> On Mon, Dec 24, 2007 at 03:04:00PM +0430, Daniel Harding wrote:
>   
>> Andrew Sackville-West wrote:
>>     
>>> On Sun, Dec 23, 2007 at 11:19:42PM -0800, Andrew Sackville-West wrote:
>>>   
>>>       
>>>> Well 16690 breaks make check. 
>>>> meanwhile I discovered that src/engine/iso-currencies-to-c isn't
>>>> getting run. At least it looks that way because its supposed to update
>>>> iso-4217-currencies.c and it doesn't. That doesn't seem to be the
>>>> source of the failure but is something to note anyway. I did manually
>>>> run iso-currencies-to-c, saw that it appeared to update
>>>> iso-4217-currencies.c properly and make check still failed. :(
>>>>     
>>>>         
>>> okay, ignore that. sorry, it does run properly.
>>>   
>>>       
>> I'm on Windows, and when I was working on that patch, it appeared that at 
>> least some times iso-4217-currencies.c was not getting regenerated 
>> properly.  I ended up just deleting iso-4217-currencies.c before rebuilding 
>> any time I changed iso-4217-currencies.scm.  So there may still be 
>> something going on here.
>>     
>
> fair enough.
>   
Found a build problem, at least on Windows.  The Makefile attempts to
create links to several .scm files from the build directory to the
source directory.  Because Windows filesystems do not support links, it
simply does a copy instead.  But, because the Makefile assumes that it
is really is a link, it does not contain dependencies between the .scm
files in the build directory and those in the source directory.  Thus
any changes made to the .scm files in the source directory will not get
propagated to the build directory after the initial copies are made.

When make checks the dependency on iso-4217-currencies.scm for
iso-4217-currencies.c, it uses the version in the build directory
instead of the version in the source directory.  Because the version in
the build directory hasn't been changed, it doesn't rebuild
iso-4217-currencies.c.

I don't know of any reason why iso-4217-currencies.scm needs to be
copied/linked, because it isn't actually used in the program at all,
just for generating the .c file.  I have attached a patch for
Makefile.am to no longer generate the link to/copy
iso-4217-currencies.scm into the build directory, and have verified that
everything still builds properly, even without iso-4217-currencies.scm
in the build directory.

There may still be similar problems related to the other .scm files, but
I don't know exactly why they are being linked to/copied (Makefile.am
seems to indicate it is for the test programs), so I can't say that for
sure.

>>>> But interestingly, Derek reverted 16619 and that causes make check to
>>>> succeed. Seems odd to me that adjusting a currency would cause 16619
>>>> to cause a test to break. So maybe there is more going on here than
>>>> meets the eye.     
>>>>         
>>> Somehow, the two lines in 16690 cause make check to fails. If you
>>> delete either line (or jsut comment the out), it passes. Leave them
>>> both in, it fails. I can't see what the heck would cause it.
>>>   
>>>       
>> Can you give more details about which test(s) are failing? 
>>     
>
> Lots. part of the engine tests.
>   
Is that test for lots, or lots of tests? :-) If I run make check in
src/engine/test, nearly everything passes for me.  I had to disable
test-book-merge because it isn't building for me, while test-date and
test-transaction-voiding both fail, but that appears to be simply
because of Windows issues rather than this patch.  Everything else
completes successfully, including test-lots.

>> Being on 
>> Windows, make check fails for me all over the place (I'm willing to help 
>> improve this, but won't be able to tackle it right away).  However, I have 
>> been able to successfully run some individual tests, so if I know where to 
>> look, I might be able to help investigate.  Does the tests also fail if you 
>> remove/comment out both currencies?
>>     
>
> it works fine in any combination other than two Afghani currencies. I
> even tried tweaking some of the values in the two lines to no avail,
> though I only tried a little. I didn't try moving one of the Afghani
> lines farther into the file in case it is somehow a problem with
> having two of the same currencies first. shrug. 
>
> I think it's a case of this change exposing problems elsewhere, maybe
> even in the tests themselves.
>
> Oh, and can you comment at all on the discussion about whether there
> should be two Afghani currencies or just one? 
>   
There is definitely only one currency in use in Afghanistan - notes from
the old currency can still be found sometimes, but they have no monetary
value.  So, ideally I think GnuCash should only have the new Afghani
(AFN).  However, if there are any users besides myself who have data
files using AFA (in which case they, like myself, are almost certainly
intending to use the new Afghani and simply are not aware of the fact
that AFA is obsolete), simply dropping it would cause problems.

I tried including only the new Afghani (AFN) in iso-4217-currencies.scm
and then to add an entry in the gnc_new_iso_codes table in
gnc-commodity.c to map the old Afghani (AFA) to the new Afghani (AFN).
However, when loading a file that used AFA, the accounts still show as
using AFA, and the Securities editor shows a bogus entry having the old
ISO 4217 code (AFA) but the new ISO 4217 code number (971) and the new
fraction of 1/1.  If I create a new file, then the Securities editor
correctly shows the new ISO 4217 code (AFN).  This isn't a huge problem
for me - I can go into my data file and manually change AFA to AFN.
However, it seems that this isn't the best way for the gnc_new_iso_codes
mechanism to work.  Still, I have attached a patch, since it seems that
other converted currencies have been handled that way.

-Daniel
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: iso-4217-build.patch
Url: http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20071225/04c415ab/attachment.pl 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: afa_to_afn.patch
Url: http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20071225/04c415ab/attachment-0001.pl