Cantera / enhancements

Repository for proposed and ongoing enhancements to Cantera
11 stars 5 forks source link

Please make build scripts compatible with Octave #123

Closed yurivict closed 2 years ago

yurivict commented 2 years ago

It seems to build with Octave. The incompatibility seems to be only in the build scripts.

The options octave_toolbox, octave_include_path, octave_library_path should probably be added.

ischoegl commented 2 years ago

@yurivict ... thank you for the suggestion. I think this should be filed as a new issue/feature request on Cantera/enhancements, where some additional context should be provided as indicated. Also, if you already have something working, a PR on Cantera/cantera would be welcome!

bryanwweber commented 2 years ago

Thanks @ischoegl I transferred it from the website repository over here to enhancements :smiley:

ischoegl commented 2 years ago

FWIW, I believe building with octave would be interesting from a unit testing perspective. As there are ongoing efforts for overhauling the MATLAB interface (see #32) the timing question here is an interesting one.

speth commented 2 years ago

I just did a quick test on Ubuntu 20.04, and this seems like it's not that bad, at least under certain circumstances. The changes I made were:

This got me through the compilation step, but the linking step fails with "undefined reference" errors to what looks to be every symbol that we use from mex.h, e.g. mxCalloc, mxCreateString, mxGetPr, etc. If I further remove the linker flag -Wl,--no-undefined, then these errors no longer appear, but I'm not sure that we won't run into a problem with this on Windows or macOS, which require all methods to symbols to be defined at link time.

After this, though, I was able to use the toolbox without any apparent issues.

However, there are two significant issues.

The first is that the work being done on #32 replaces the use of mex with loadlibrary and calllib, which are apparently not implemented in Octave.

The second is that Octave is licensed under the GPL, which says that linking to a GPL package creates a derivative work that must be licensed under the same terms. However, Cantera is licensed under a BSD-style license, and cannot be relicensed under the GPL, so I think if you compile Cantera using Octave's mex.h, you create a package that cannot be redistributed without violating one license or the other.

bryanwweber commented 2 years ago

A quick search turned up a packaging standard for Octave: https://github.com/gnu-octave/pkg#creating-packages They list that the license of the compiled package code must be GPL-compatible. I suppose it would be possible to write a (L?)GPL-licensed intermediary interface that links to Cantera's clib. That should resolve the licensing issue, if I understand correctly, at the consequence of another layer of indirection and another set of functions to maintain.

yurivict commented 2 years ago

The second is that Octave is licensed under the GPL, which says that linking to a GPL package creates a derivative work that must be licensed under the same terms.

One way to solve this is is to delegate the decision to users. Make headers/libraries overridable by users, since only headers and libraries are different. When the user would choose to build Cantera with Octave for personal use - this would be purely user's responsibility and would likely not violate any licensing terms.

yurivict commented 2 years ago

This got me through the compilation step, but the linking step fails with "undefined reference" errors to what looks to be every symbol that we use from mex.h, e.g. mxCalloc, mxCreateString, mxGetPr, etc.

The liboctinterp.so library exports these symbols. You should probably add it.

ischoegl commented 2 years ago

Thanks @speth for testing, that is very interesting. While it should be possible to resolve the licensing issue by not explicitly packaging things together, I believe the other issue that was mentioned may render this moot:

[…] work being done on #32 replaces the use of mex with loadlibrary and calllib, which are apparently not implemented in Octave.

while I’m not sure that #32 will be ready for the 2.6 release, any discussion should probably assume that the old interface is going away hopefully sooner than later. Whether or not octave can be supported will depend on the outcome of #32 (or the availability of interfaces mentioned above).

bryanwweber commented 2 years ago

Whether or not octave can be supported will depend on the outcome of #32 (or the availability of interfaces mentioned above).

@ischoegl Well, we could rename the existing interface to an Octave interface and continue maintaining it. I'm not familiar with the OOP structures in Octave, so I'm not aware if they have a "new" or "old" style the way Matlab does. Also, this is not a very highly requested feature, so to the extent that it takes up maintainer work cycles, it may not be worth the effort. Indeed, this request came up in the context of installation instructions including a warning that the MATLAB package is not available, not as a specific feature request for compatibility.

ischoegl commented 2 years ago

Well, we could rename the existing interface to an Octave interface and continue maintaining it […] Also, this is not a very highly requested feature …

My personal take on this is that we should plan for a clean cut (I’m not sure how far along #32 is, but ideally 3.0 should only have the new MATLAB interface).

bryanwweber commented 2 years ago

My personal take on this is that we should plan for a clean cut

Sure, a clean cut for the MATLAB interface. But if the existing code is separately useful as an Octave interface, and there isn't a way to use the new MATLAB interface in Octave, and there isn't a more idiomatic Octave-specific way of creating the package, I think it could make sense to keep it around.

speth commented 2 years ago

I would strongly oppose maintaining two separate interfaces for Matlab and Octave. Matlab is already one of the less-used Cantera interfaces, and inquiries about Octave support have been few and far between. The only way I see that an Octave interface being viable is if it is more or less identical to the Matlab interface.

It looks like Octave has some support for classdef-style classes, although there are limitations. I don't know if any of those limitations matter for the new interface that @ssun30 is working on, but I that would be helpful to know. I think transitioning to the classdef-style classes is one of the significant benefits of this project, so I wouldn't want to have to abandon that aspect of the toolbox upgrade.

Whether the Matlab interface is based on mex or loadlibrary is in some ways less critical. I think there are components that will be hard to implement using the loadlibrary approach (namely, setting up Matlab-based functions that need to be called from Cantera's C++ functions, which is needed in a limited sense for the Func1 capability and more broadly for the features proposed in #79 and partially implemented for Python in Cantera/cantera#1003). So if the Matlab toolbox ends up sticking with mex, then having some sort of Octave support would probably make sense. What I would like to avoid, though, is introducing this support now, and then removing it as soon as a new Matlab toolbox based on loadlibrary is finished.

Regarding the licensing issue, the Octave FAQ provides an exception to the linking restriction for the mex interface: http://wiki.octave.org/FAQ#If_I_write_code_using_Octave_do_I_have_to_release_it_under_the_GPL.3F:

Code written using Octave's implementation of the Matlab MEX interface may be released under the terms of whatever license you choose, provided that the following conditions are met:

  1. The MEX file may not use any bindings that are specific to Octave, it has to use the MEX interface only. In other words, it should be possible in principle to use the MEX file with other programs that implement the MEX interface (e.g., Matlab). For example including an Octave header file or calling an Octave function within the MEX file, that is not related with Octave's implementation of the MEX interface make the MEX file a derivative work of Octave and has therefore to be released under terms that are compatible with the GPL.
  2. The MEX file may not be distributed together with Octave in such a way that they effectively create a single work. For example, you should not distribute the MEX file and Octave together in a single package such that Octave automatically loads and runs the MEX file when it starts up. There are other possible ways to effectively create a single work.

So I guess for our purposes there's no barrier to distributing compiled versions of the Octave toolbox under Cantera's existing BSD license.

mefuller commented 2 years ago

I'll just chime in to say that this is something that interests me, but that I also appreciate that it is of limited utility to end users and that I doubt I can contribute much technically. I would, however, be happy to try helping on development and testing, especially if anyone can help give a few pointers as questions arise.

ischoegl commented 2 years ago

Now that Cantera/cantera#1182 (which addresses #32) is officially ready for review, I wanted to follow up here as Octave may still be a viable option to cover the MATLAB toolbox with a routine CI job (fwiw, I'd only consider it for the new toolbox). Tagging @ssun30 and @rwest ...

speth commented 2 years ago

As was mentioned before, the toolbox implemented in https://github.com/Cantera/cantera/pull/1182 is completely incompatible with Octave, due to its use of calllib as the mechanism for calling Cantera's C++ functions. The only way to support Octave would be to stick with the mex-based interface.

ischoegl commented 2 years ago

@speth ... thanks for triggering my memory! It's been a while and I did not recall this critical detail - my recollection was that it wouldn't be a complete roadblock, but I was apparently mistaken. Based on your assessment, my comment can certainly be ignored :smile:

As support for the old toolbox will likely be dropped in the foreseeable future, there essentially does not appear to be a path to support octave down the line.

bryanwweber commented 2 years ago

As support for the old toolbox will likely be dropped in the foreseeable future, there essentially does not appear to be a path to support octave down the line.

Agreed, with the nuance that if a community member is willing to step up as a maintainer of this interface, I would consider it. But that person should be willing and able to support the interface in the long term, not just get it working and disappear 😄

ischoegl commented 2 years ago

@bryanwweber … agreed. For documentation, here’s an octave bug report for loadlibrary: there apparently has been discussion on this off and on, but I do not sense an urgency for a resolution. Fwiw, there appears to be a MATLAB runner on GH, which will eliminate the need for alternative CI options.

Closing.