Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Swig 4.0 makes incompatible changes to MMCoreJ API (and also fails build) #37

Open
marktsuchida opened this issue Sep 3, 2020 · 8 comments
Open

Swig 4.0 makes incompatible changes to MMCoreJ API (and also fails build) #37

marktsuchida opened this issue Sep 3, 2020 · 8 comments
Assignees
@marktsuchida

Comments

@marktsuchida
Copy link
Member

This is a continuation of micro-manager/micro-manager#838.

Quoting from the Swig Changelog, in 4.0.0 there is the following entry:

2019-02-28: wsfulton
            [Java] std::vector improvements for types that do not have a default constructor.

            The std::vector wrappers have been changed to work by default for elements that are
            not default insertable, i.e. have no default constructor. This has been achieved by
            not wrapping:

              vector(size_type n);

            Previously the above had to be ignored via %ignore.

            If the above constructor is still required it can be added back in again via %extend:

              %extend std::vector {
                vector(size_type count) { return new std::vector< T >(count); }
              }

            Alternatively, the following wrapped constructor could be used as it provides near-enough
            equivalent functionality:

              vector(jint count, const value_type& value);

            *** POTENTIAL INCOMPATIBILITY ***

This suggests that it might be possible to have code that works across Swig versions.
@marktsuchida marktsuchida self-assigned this Sep 3, 2020
@nicost nicost transferred this issue from micro-manager/micro-manager Aug 2, 2021
@JiangXL
Copy link
Contributor

JiangXL commented Aug 4, 2021

May I ask how it is going with Swig 4.0?

@marktsuchida
Copy link
Member Author

marktsuchida commented Aug 4, 2021 •
edited
Loading

Let me try to at least make a to-do list for this.

Below are the relevant changes (swig <4 to swig 4) that we need to deal with (generated using japicmp and filtered).

As far as I can tell, they result from 3 changes to SWIG's Java support (from the changelog linked above):

  • 2017-05-26 "Extend from java.util.AbstractList<> and implement java.util.RandomAccess for std::vector wrappers"
  • 2019-02-14 "std::map wrappers have been modified. Now the Java proxy class extends java.util.AbstractMap."
  • 2019-02-28 "std::vector improvements for types that do not have a default constructor"

Of the API changes, we can accept the ones labeled NEW INTERFACE, MODIFIED SUPERCLASS (from Object), NEW CONSTRUCTOR, and NEW METHOD. We can also accept the single NEW CLASS (StrMap$Iterator). (But it does mean we should switch to requiring SWIG 4 once we enable it, or else the API will become incompatible between copies of MMCoreJ.jar built by different SWIG versions.)

Among the backward-incompatible changes, there is not much we can do about the MODIFIED METHODs, where the return value of get() got changed to the boxed types and the return value of size() got changed from long to int. These can break existing user code, but hopefully not that frequently; the only failure mode is a compile error, so people will easily see how their code needs to change.

That leaves us with the REMOVED CONSTRUCTORs and REMOVED METHODs. More about these in my next comment.

***! MODIFIED CLASS: PUBLIC mmcorej.BooleanVector  (not serializable)
	+++  NEW INTERFACE: java.util.RandomAccess
	+++  NEW INTERFACE: java.util.List
	+++  NEW INTERFACE: java.util.Collection
	***  MODIFIED SUPERCLASS: java.util.AbstractList (<- java.lang.Object)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) BooleanVector(long)
	+++  NEW CONSTRUCTOR: PUBLIC(+) BooleanVector(int, boolean)
	+++  NEW CONSTRUCTOR: PUBLIC(+) BooleanVector(java.lang.Iterable)
	+++  NEW CONSTRUCTOR: PUBLIC(+) BooleanVector(mmcorej.BooleanVector)
	+++  NEW CONSTRUCTOR: PUBLIC(+) BooleanVector(boolean[])
	---! REMOVED METHOD: PUBLIC(-) void add(boolean)
	+++  NEW METHOD: PUBLIC(+) boolean add(java.lang.Boolean)
	+++  NEW METHOD: PUBLIC(+) void add(int, java.lang.Boolean)
	***! MODIFIED METHOD: PUBLIC java.lang.Boolean (<-boolean) get(int)
	+++  NEW METHOD: PUBLIC(+) java.lang.Boolean remove(int)
	+++  NEW METHOD: PROTECTED(+) void removeRange(int, int)
	---! REMOVED METHOD: PUBLIC(-) void set(int, boolean)
	+++  NEW METHOD: PUBLIC(+) java.lang.Boolean set(int, java.lang.Boolean)
	***! MODIFIED METHOD: PUBLIC int (<-long) size()
***! MODIFIED CLASS: PUBLIC mmcorej.CharVector  (not serializable)
	+++  NEW INTERFACE: java.util.RandomAccess
	+++  NEW INTERFACE: java.util.List
	+++  NEW INTERFACE: java.util.Collection
	***  MODIFIED SUPERCLASS: java.util.AbstractList (<- java.lang.Object)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) CharVector(long)
	+++  NEW CONSTRUCTOR: PUBLIC(+) CharVector(int, char)
	+++  NEW CONSTRUCTOR: PUBLIC(+) CharVector(mmcorej.CharVector)
	+++  NEW CONSTRUCTOR: PUBLIC(+) CharVector(java.lang.Iterable)
	+++  NEW CONSTRUCTOR: PUBLIC(+) CharVector(char[])
	---! REMOVED METHOD: PUBLIC(-) void add(char)
	+++  NEW METHOD: PUBLIC(+) boolean add(java.lang.Character)
	+++  NEW METHOD: PUBLIC(+) void add(int, java.lang.Character)
	***! MODIFIED METHOD: PUBLIC java.lang.Character (<-char) get(int)
	+++  NEW METHOD: PUBLIC(+) java.lang.Character remove(int)
	+++  NEW METHOD: PROTECTED(+) void removeRange(int, int)
	---! REMOVED METHOD: PUBLIC(-) void set(int, char)
	+++  NEW METHOD: PUBLIC(+) java.lang.Character set(int, java.lang.Character)
	***! MODIFIED METHOD: PUBLIC int (<-long) size()
***! MODIFIED CLASS: PUBLIC mmcorej.DoubleVector  (not serializable)
	+++  NEW INTERFACE: java.util.RandomAccess
	+++  NEW INTERFACE: java.util.List
	+++  NEW INTERFACE: java.util.Collection
	***  MODIFIED SUPERCLASS: java.util.AbstractList (<- java.lang.Object)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) DoubleVector(long)
	+++  NEW CONSTRUCTOR: PUBLIC(+) DoubleVector(mmcorej.DoubleVector)
	+++  NEW CONSTRUCTOR: PUBLIC(+) DoubleVector(int, double)
	+++  NEW CONSTRUCTOR: PUBLIC(+) DoubleVector(double[])
	+++  NEW CONSTRUCTOR: PUBLIC(+) DoubleVector(java.lang.Iterable)
	---! REMOVED METHOD: PUBLIC(-) void add(double)
	+++  NEW METHOD: PUBLIC(+) boolean add(java.lang.Double)
	+++  NEW METHOD: PUBLIC(+) void add(int, java.lang.Double)
	***! MODIFIED METHOD: PUBLIC java.lang.Double (<-double) get(int)
	+++  NEW METHOD: PUBLIC(+) java.lang.Double remove(int)
	+++  NEW METHOD: PROTECTED(+) void removeRange(int, int)
	---! REMOVED METHOD: PUBLIC(-) void set(int, double)
	+++  NEW METHOD: PUBLIC(+) java.lang.Double set(int, java.lang.Double)
	***! MODIFIED METHOD: PUBLIC int (<-long) size()
***! MODIFIED CLASS: PUBLIC mmcorej.LongVector  (not serializable)
	+++  NEW INTERFACE: java.util.RandomAccess
	+++  NEW INTERFACE: java.util.List
	+++  NEW INTERFACE: java.util.Collection
	***  MODIFIED SUPERCLASS: java.util.AbstractList (<- java.lang.Object)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) LongVector(long)
	+++  NEW CONSTRUCTOR: PUBLIC(+) LongVector(int[])
	+++  NEW CONSTRUCTOR: PUBLIC(+) LongVector(mmcorej.LongVector)
	+++  NEW CONSTRUCTOR: PUBLIC(+) LongVector(java.lang.Iterable)
	+++  NEW CONSTRUCTOR: PUBLIC(+) LongVector(int, int)
	---! REMOVED METHOD: PUBLIC(-) void add(int)
	+++  NEW METHOD: PUBLIC(+) boolean add(java.lang.Integer)
	+++  NEW METHOD: PUBLIC(+) void add(int, java.lang.Integer)
	***! MODIFIED METHOD: PUBLIC java.lang.Integer (<-int) get(int)
	+++  NEW METHOD: PUBLIC(+) java.lang.Integer remove(int)
	+++  NEW METHOD: PROTECTED(+) void removeRange(int, int)
	---! REMOVED METHOD: PUBLIC(-) void set(int, int)
	+++  NEW METHOD: PUBLIC(+) java.lang.Integer set(int, java.lang.Integer)
	***! MODIFIED METHOD: PUBLIC int (<-long) size()
***! MODIFIED CLASS: PUBLIC mmcorej.StrMap  (not serializable)
	+++  NEW INTERFACE: java.util.Map
	***  MODIFIED SUPERCLASS: java.util.AbstractMap (<- java.lang.Object)
	+++  NEW METHOD: PUBLIC(+) boolean containsKey(java.lang.Object)
	---! REMOVED METHOD: PUBLIC(-) void del(java.lang.String)
	---! REMOVED METHOD: PUBLIC(-) boolean empty()
	+++  NEW METHOD: PUBLIC(+) java.util.Set entrySet()
	---! REMOVED METHOD: PUBLIC(-) java.lang.String get(java.lang.String)
	+++  NEW METHOD: PUBLIC(+) java.lang.String get(java.lang.Object)
	---! REMOVED METHOD: PUBLIC(-) boolean has_key(java.lang.String)
	+++  NEW METHOD: PUBLIC(+) boolean isEmpty()
	+++  NEW METHOD: PUBLIC(+) java.lang.String put(java.lang.String, java.lang.String)
	+++  NEW METHOD: PUBLIC(+) java.lang.String remove(java.lang.Object)
	---! REMOVED METHOD: PUBLIC(-) void set(java.lang.String, java.lang.String)
	***! MODIFIED METHOD: PUBLIC int (<-long) size()
+++  NEW CLASS: PROTECTED(+) STATIC(+) mmcorej.StrMap$Iterator  (not serializable)
	+++  NEW SUPERCLASS: java.lang.Object
	+++  NEW FIELD: PROTECTED(+) boolean swigCMemOwn
	+++  NEW CONSTRUCTOR: PROTECTED(+) StrMap$Iterator(long, boolean)
	+++  NEW METHOD: PUBLIC(+) void delete()
	+++  NEW METHOD: PROTECTED(+) void finalize()
	+++  NEW METHOD: PROTECTED(+) STATIC(+) long getCPtr(mmcorej.StrMap$Iterator)
***! MODIFIED CLASS: PUBLIC mmcorej.StrVector  (not serializable)
	+++  NEW INTERFACE: java.util.RandomAccess
	+++  NEW INTERFACE: java.util.List
	+++  NEW INTERFACE: java.util.Collection
	***  MODIFIED SUPERCLASS: java.util.AbstractList (<- java.lang.Object)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) StrVector(long)
	+++  NEW CONSTRUCTOR: PUBLIC(+) StrVector(java.lang.Iterable)
	+++  NEW CONSTRUCTOR: PUBLIC(+) StrVector(java.lang.String[])
	+++  NEW CONSTRUCTOR: PUBLIC(+) StrVector(mmcorej.StrVector)
	+++  NEW CONSTRUCTOR: PUBLIC(+) StrVector(int, java.lang.String)
	***! MODIFIED METHOD: PUBLIC boolean (<-void) add(java.lang.String)
	+++  NEW METHOD: PUBLIC(+) void add(int, java.lang.String)
	+++  NEW METHOD: PUBLIC(+) java.lang.String remove(int)
	+++  NEW METHOD: PROTECTED(+) void removeRange(int, int)
	***! MODIFIED METHOD: PUBLIC java.lang.String (<-void) set(int, java.lang.String)
	***! MODIFIED METHOD: PUBLIC int (<-long) size()
***! MODIFIED CLASS: PUBLIC mmcorej.UnsignedVector  (not serializable)
	+++  NEW INTERFACE: java.util.RandomAccess
	+++  NEW INTERFACE: java.util.List
	+++  NEW INTERFACE: java.util.Collection
	***  MODIFIED SUPERCLASS: java.util.AbstractList (<- java.lang.Object)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) UnsignedVector(long)
	+++  NEW CONSTRUCTOR: PUBLIC(+) UnsignedVector(java.lang.Iterable)
	+++  NEW CONSTRUCTOR: PUBLIC(+) UnsignedVector(mmcorej.UnsignedVector)
	+++  NEW CONSTRUCTOR: PUBLIC(+) UnsignedVector(long[])
	+++  NEW CONSTRUCTOR: PUBLIC(+) UnsignedVector(int, long)
	---! REMOVED METHOD: PUBLIC(-) void add(long)
	+++  NEW METHOD: PUBLIC(+) boolean add(java.lang.Long)
	+++  NEW METHOD: PUBLIC(+) void add(int, java.lang.Long)
	***! MODIFIED METHOD: PUBLIC java.lang.Long (<-long) get(int)
	+++  NEW METHOD: PUBLIC(+) java.lang.Long remove(int)
	+++  NEW METHOD: PROTECTED(+) void removeRange(int, int)
	---! REMOVED METHOD: PUBLIC(-) void set(int, long)
	+++  NEW METHOD: PUBLIC(+) java.lang.Long set(int, java.lang.Long)
	***! MODIFIED METHOD: PUBLIC int (<-long) size()

@marktsuchida
Copy link
Member Author

Here are the changes that we probably want to avoid (and probably can) (extracted and reordered from above):

***! MODIFIED CLASS: PUBLIC mmcorej.BooleanVector  (not serializable)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) BooleanVector(long)
***! MODIFIED CLASS: PUBLIC mmcorej.CharVector  (not serializable)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) CharVector(long)
***! MODIFIED CLASS: PUBLIC mmcorej.DoubleVector  (not serializable)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) DoubleVector(long)
***! MODIFIED CLASS: PUBLIC mmcorej.LongVector  (not serializable)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) LongVector(long)
***! MODIFIED CLASS: PUBLIC mmcorej.StrVector  (not serializable)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) StrVector(long)
***! MODIFIED CLASS: PUBLIC mmcorej.UnsignedVector  (not serializable)
	---! REMOVED CONSTRUCTOR: PUBLIC(-) UnsignedVector(long)

***! MODIFIED CLASS: PUBLIC mmcorej.BooleanVector  (not serializable)
	---! REMOVED METHOD: PUBLIC(-) void add(boolean)
	---! REMOVED METHOD: PUBLIC(-) void set(int, boolean)
***! MODIFIED CLASS: PUBLIC mmcorej.CharVector  (not serializable)
	---! REMOVED METHOD: PUBLIC(-) void add(char)
	---! REMOVED METHOD: PUBLIC(-) void set(int, char)
***! MODIFIED CLASS: PUBLIC mmcorej.DoubleVector  (not serializable)
	---! REMOVED METHOD: PUBLIC(-) void add(double)
	---! REMOVED METHOD: PUBLIC(-) void set(int, double)
***! MODIFIED CLASS: PUBLIC mmcorej.LongVector  (not serializable)
	---! REMOVED METHOD: PUBLIC(-) void add(int)
	---! REMOVED METHOD: PUBLIC(-) void set(int, int)
***! MODIFIED CLASS: PUBLIC mmcorej.UnsignedVector  (not serializable)
	---! REMOVED METHOD: PUBLIC(-) void add(long)
	---! REMOVED METHOD: PUBLIC(-) void set(int, long)

***! MODIFIED CLASS: PUBLIC mmcorej.StrMap  (not serializable)
	---! REMOVED METHOD: PUBLIC(-) void del(java.lang.String)
	---! REMOVED METHOD: PUBLIC(-) boolean empty()
	---! REMOVED METHOD: PUBLIC(-) java.lang.String get(java.lang.String)
	---! REMOVED METHOD: PUBLIC(-) boolean has_key(java.lang.String)
	---! REMOVED METHOD: PUBLIC(-) void set(java.lang.String, java.lang.String)

So the remaining job is to reintroduce the above constructors and methods by editing MMCoreJ.i, and to switch all of our automated builds to use SWIG 4.x exclusively. We should also cause the build to fail when attempting to use SWIG <4.

@marktsuchida marktsuchida changed the title MMCoreJ doesn't build with Swig 4.0 Swig 4.0 makes incompatible changes to MMCoreJ API (and also fails build) Aug 4, 2021
@marktsuchida
Copy link
Member Author

For anybody seeing this before we fix the above API issues: please continue to build using SWIG 3.x for the time being.

@fabio-echegaray fabio-echegaray mentioned this issue Feb 11, 2022
Closed
@marktsuchida
Copy link
Member Author

Something I do not address above is whether the API is equivalent between a Windows build and macOS/Linux build. This should also be checked.

@marktsuchida marktsuchida mentioned this issue Feb 11, 2022
Closed
@marktsuchida marktsuchida mentioned this issue Feb 21, 2022
Open
2 tasks
@ianhi ianhi mentioned this issue Mar 18, 2022
Open
@marktsuchida
Copy link
Member Author

Not sure why this never occurred to me before, but we should investigate if simply replacing long with int in the MMCore functions will solve the problem, at least partially.

On the C++ side, int is always 4 bytes on all the platforms we care about. long is 4 bytes on Windows (LLP64) and 8 bytes on 64-bit macOS/Linux (LP64). So MMCore was written to expect long to be 4 bytes, and we don't expect long values to exceed the int range (not that we do anything about numerical overflow anyway).

The problem we are having is that C++ long, which SWIG 3 always seems to have mapped to Java int, is now mapped to Java long by SWIG 4 on LP64 platforms only, whereas we'd like it to stay mapped to Java int.

We don't need to consider pymmcore because Python just has arbitrary-precision integers.

So replacing (almost) all long parameters and return values with int might not break anything, even for C++ user code, save for possible warnings on LP64 platforms.

@nicost
Copy link
Member

nicost commented Jun 15, 2022

I believe the other issue is with StrVector and friends. If I remember correctly, Swig 4.0 maps those into native Java classes (much nicer, but will need extensive changes to the Java code.

@marktsuchida
Copy link
Member Author

Right. But I think we can solve the *Vector problem once the long problem is solved: we just need to create a Java StrVector that implements List<String> but also has the same methods as the SWIG 3 StrVector. Then,

  • Wrapped functions taking a List<String> parameter will have no problem when passed a StrVector
  • We need to make wrapped functions that return std::vector<std::string> return StrVector for backward compatibility, but user code can start treating the return value as List<String>. This is annoying but I think doable.

@ianhi ianhi mentioned this issue Jul 12, 2022
Merged
@marktsuchida marktsuchida mentioned this issue Sep 27, 2022
Closed
@marktsuchida marktsuchida mentioned this issue Nov 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@marktsuchida marktsuchida

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@marktsuchida @nicost @JiangXL