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

[Derek Atkins]

Quoting Daniel Harding <dharding at gmail.com>:

> 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.

Correct.  On Windows you should use a separate build-dir, or you
should "make distclean" between each build.  I'm pretty sure
that the "install.sh" scripts do this automatically for you.  Are
you using the win32 build scripts or are you building it yourself
by hand?

> 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.

Hmm....

> 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.

True, for THAT case you could just run it specifically out of
$(srcdir).

> 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.

Yes, it's because you need to run "make check" in the build tree,
and it's expecting a specific directory hierarchy in the build..
and since it cannot make symlinks it has to copy them.

>> Lots. part of the engine tests.
>>
> Is that test for lots, or lots of tests? :-) If I run make check in

I hope this is a joke.  "test-lots".

> 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.

You shouldn't have to disable any tests.  If you do, then there's
a problem.  "make check" should pass on all platforms, and I know
that Christian spent much time working on that.

>> 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.

There's no need to drop it.  GnuCash has a conversion routine that
will automatically convert from the old to the new currency.

> 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.

Thanks..

> -Daniel

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available