Ah yes - thanks for keeping your eye on this Mark. When I looked over
this change I missed the point that jniast carries its own (rather
old) AST source files round with it in src/ast, and does not use an
external AST library. So as you say there is no need to make any
changes to jniast.
Having said that, if the src/ast files *had* been updated to the new
versions, Graham's changes to (e.g.) src/jni/SlaMap.c do not change
the API or functionality of the Java_uk_ac_starlink_ast_SlaMap_add
function so I'm not sure I understand why it would break anything.
However, as you say, it's probably better to back out of these
changes. Especially given that we've not incorporated any other AST
changes into jniast for a long time. At some point it would be nice to
bring jniast up to date wrt AST, but there seems to be little demand
for it (as far as I can tell), so motivation is low.
David
On 21 July 2017 at 10:21, Mark Taylor <[log in to unmask]> wrote:
> Graham,
>
> we need to discuss this change further.
>
> On their own, changes to the jniast/src/jni/*.c files do not
> affect a normal (ant build install) starjava build at all.
> Given that, the changes at this commit to the AstTest file
> (apparently requiring some new failures that show up following
> the *Map.c changes you've made) now cause failures in a normal
> build-install-test sequence.
>
> But assuming that you're doing a compilation of the C source files
> (src/jni/SlaMap.c et al.) so that these changes do modify the
> runtime behaviour in your environment, you can't just change them
> without making corresponding changes to the corresponding java source
> files (src/main/uk/ac/starlink/ast/SlaMap.java et al.)
> which in turn are (mostly) auto-generated by the perl files
> (src/perl/SlaMap.pl et al.). Doing so will probably result in
> core dumps at runtime (a very bad thing in java).
> As well as that, if the JNI interface changes, as implied by
> changes to the jni/*.c files, then the src/lib/*/jniast_libs.jar
> native files need to be recompiled and committed for all
> supported architectures.
>
> The background describing how this package hangs together is
> discussed in src/jniast/README.developer. It's fairly complicated.
>
> I don't really mind what happens on branch 2017A, but on master
> this either needs to be patched up properly, or backed out of.
> If there's some strong reason why this change has to be made,
> I'll take a look at what you want to do. But if not, I suggest
> we leave it as it was - JNIAST is a long way behind AST anyway
> and it's complicated enough that leaving well alone is a good
> strategy.
>
> Mark
>
>
> On Fri, 21 Jul 2017, Graham Bell wrote:
>
>> Branch: refs/heads/master
>> Home: https://github.com/Starlink/starjava
>> Commit: 345e9d6364dd611475ba151617d895f0258a971a
>> https://github.com/Starlink/starjava/commit/345e9d6364dd611475ba151617d895f0258a971a
>> Author: Graham Bell <[log in to unmask]>
>> Date: 2017-07-20 (Thu, 20 Jul 2017)
>>
>> Changed paths:
>> M jniast/src/jni/SlaMap.c
>> M jniast/src/jni/SpecMap.c
>> M jniast/src/jni/TimeMap.c
>> M jniast/src/testcases/uk/ac/starlink/ast/AstTest.java
>>
>> Log Message:
>> -----------
>> jniast: Update astSlaAdd, astSpecAdd & astTimeAdd
>>
>> Add the new "nargs" argument. This is currently derived automatically
>> from the length of the argument array given. The alternative would
>> be (as in the C-interface) to have the user provide the number of
>> arguments manually as an extra argument.
>>
>>
>>
>
> --
> Mark Taylor Astronomical Programmer Physics, Bristol University, UK
> [log in to unmask] +44-117-9288776 http://www.star.bris.ac.uk/~mbt/
|