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.

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

Please make build scripts compatible with Octave #123

Closed
yurivict opened this issue Dec 18, 2021 · 18 comments
Closed

Please make build scripts compatible with Octave #123

yurivict opened this issue Dec 18, 2021 · 18 comments
Labels
feature-request New feature request

Comments

@yurivict
Copy link

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
Copy link
Member

@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 bryanwweber transferred this issue from Cantera/cantera-website Dec 18, 2021
@bryanwweber bryanwweber added the feature-request New feature request label Dec 18, 2021
@bryanwweber
Copy link
Member

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

@ischoegl
Copy link
Member

ischoegl commented Dec 18, 2021

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
Copy link
Member

speth commented Dec 19, 2021

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:

  • edit SConstruct to delete the check for mex.h relative to the matlab_path
  • Edit src/matlab/SConscript and change the list of libraries that are linked to to specify octave instead of mx, mex, and mat
  • Set mexSuffix to just mex

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
Copy link
Member

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
Copy link
Author

yurivict commented Dec 19, 2021

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
Copy link
Author

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
Copy link
Member

ischoegl commented Dec 19, 2021

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
Copy link
Member

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
Copy link
Member

ischoegl commented Dec 19, 2021

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
Copy link
Member

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
Copy link
Member

speth commented Dec 20, 2021

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
Copy link

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
Copy link
Member

ischoegl commented Feb 16, 2022

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
Copy link
Member

speth commented Feb 16, 2022

As was mentioned before, the toolbox implemented in Cantera/cantera#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
Copy link
Member

ischoegl commented Feb 16, 2022

@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 😄

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature request
Projects
None yet
Development

No branches or pull requests

5 participants