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

Let gmtmex be a regular GMT supplement #4245

Open
PaulWessel opened this issue Sep 22, 2020 · 34 comments
Open

Let gmtmex be a regular GMT supplement #4245

PaulWessel opened this issue Sep 22, 2020 · 34 comments
Assignees

Comments

@PaulWessel
Copy link
Member

Description of the desired feature

Unlike PyGMT and GMT.jl, the GMT/MEX toolbox requires compiled interface code. We currently have a separate repo with a poorly controlled Xcode debug framework hacked form the GMT setup. Debugging gmt/mex from Matlab *.m files requires a bit of gymnastics in taht we must supply the GMT library as dependency to the gmtmex build but the run that gmtmex build in Xcode, rather than the GMT C lib which we do for PyGMT and GMT.jl. It thus makes sense to keep gmtmex under GMT so that it can simply inherit all the well-constrained CMake setup.

I expect we handle this by adding a parameter or two to the ConfigUserAdvancedTemplate.cmake setup since we will need to find the MATLAB mex libraries and includes during build. I will therefore make a branch to test this out and I may have questions since some configuring is only conditionally needed depending on GMT_MEX is true of similar.

@PaulWessel
Copy link
Member Author

Once question for @seisman may be is there any way, or point, to bring in the git history we have for gmtmex, or is that simply lost? I do not think this is a problem since the old stuff is very wrong...

@seisman
Copy link
Member

seisman commented Sep 22, 2020

Once question for @seisman may be is there any way, or point, to bring in the git history we have for gmtmex, or is that simply lost?

Do you mean that "gmtmex.c" was in the repository and then deleted? I can't find it in the git history.

@PaulWessel
Copy link
Member Author

No, I mean the separate repo gmtmex has gmtmex.c and gmtmex_parser.[ch] and its own history separate from the gmt repo, and I assume there is no simple way to include that. So we just go from master of gmtmex.

@seisman
Copy link
Member

seisman commented Sep 22, 2020

We can include the gmtmex repository into the gmt repository using git submodules, but I don't think it's worth the complexity.

@PaulWessel
Copy link
Member Author

Yep, me neither. The history has no value I think since we had to make lots of changes as we learned how to work with the API. Now it is more stable and we have not done too much on the gmtmex C side, mostly in the GMT C like for PyGMT etc.

@PaulWessel
Copy link
Member Author

So the trick will be to have the compilation in that supplement run the equivalent of what the makefile does:

$(PROGS):	$(PROGS_O) $(LIB_O)
		$(MEX_BLD) $(FLAGS) $(PROGS_O) $(LIB_O) $(ALLLIB) $(MEX_OUT) gmtmex.$(MEX_EXT)

where we have various CMake variables set in Advanced such as

set (MEX_BLD "xcrun clang -undefined error -arch x86_64 -bundle -DGMT_MATLAB ${MATLAB_FLAGS})

and ALLLIB = $(GMT_LIB) $(MEX_LIB)
probably should just be $(MEX_LIB) since we are building GMT and it is already there.

In your CMakeLists.txt loop over all the supplements you find, I think that is where we need to exclude including the gmtmex by testing GMT_BUILD_GMTMEX is true/false?

@PaulWessel
Copy link
Member Author

So seems to me a few complications:

  1. The gmtmex_parser.[ch] is just some glue that can go into the supplements.so file but it need to add those links to the -L${MATLAB}/bin/${MATLAB_MEX} -lmx -lmex but also needs include dir -I${MATLAB}/extern/include. This is regular build with an extra library dependency I think. So hopefully not to hard to do and include in supplements.so if GMT_BUILD_GMTMEX si true.
  2. Building gmtmex.$(MEX_EXT) means we must run a custom command with a different set of file extension. This is an application (well, mex program), similar to gmt built from gmt.c, but can be run from the matlab command prompt (well, we call it via a matalb *.m script called gmt.m). This seems tricker to me. I wonder if perhaps just the glue goes in gmtmex and then the gmtmex.c executable is built elsewhere?

@seisman
Copy link
Member

seisman commented Sep 22, 2020

I'm not a matlab user and have limited matlab/mex knowledge. I'm wondering if it's a good idea to include gmtmex as a GMT supplement.

When homebrew, macports and other Linux distros build the GMT package, they usually don't have matlab installed, so the gmt package doesn't have the gmtmex interface enabled. For those users, if they want to use/try GMT in Matlab, they need to either build GMT source codes or use the GMT official bundle/installer. Am I understanding it correctly?

@PaulWessel
Copy link
Member Author

Yes you are. I think perhaps the goal is for us to build Bundle installer that contains the gmtmex or not. Right now it is a nightmare operation. And I think without it being in the installer (it is in Win, but not in Bundle) it is too hard to have otehrs use it. On Linux I think they would need to build from source like we do - but for now I am aiming for macOS only.

@seisman
Copy link
Member

seisman commented Sep 22, 2020

the goal is for us to build Bundle installer that contains the gmtmex or not. Right now it is a nightmare operation.

The bundle also doesn't work for PyGMT (maybe also GMT.jl). To me, the problem may be some missing settings for the bundles.

@PaulWessel
Copy link
Member Author

OK, good to know (well, heard that before), perhaps something to keep an eye on in this. I think the line that builds the gmt executable is this section in src/CMakeLists.txt:

##
## Add executables
##
# build targets for standalone gmt target and demos
string (REPLACE ".c" "" _gmt_progs "gmt.c;${GMT_DEMOS_SRCS}")
foreach (_gmt_prog ${_gmt_progs})
	add_executable (${_gmt_prog} ${_gmt_prog}.c)
	target_link_libraries (${_gmt_prog} gmtlib)
endforeach (_gmt_prog)

# Rename gmt target to prevent version clash
set_target_properties (gmt PROPERTIES OUTPUT_NAME gmt${GMT_INSTALL_NAME_SUFFIX})

and the look is there to add test and demo executables (?). So maybe we need to do something like

	add_executable (gmtmex.c)
	target_link_libraries (gmtmex gmtlib)

but somehow deal with the change of extension.

@PaulWessel
Copy link
Member Author

Getting closer with

set (SUPPL_NAME gmtmex)
set (SUPPL_HEADERS gmtmex.h)
set (SUPPL_LIB_SRCS gmtmex_parser.c)
set (SUPPL_EXAMPLE_FILES README.gmtmex)

# Add the standalone mexFunction
add_executable (gmtmex gmtmex.c)
target_link_libraries (gmtmex gmtlib)
# Rename gmt target to prevent version clash
set_target_properties (gmtmex PROPERTIES OUTPUT_NAME gmtmex.${MEX_EXT})

# install gmt/mex external function interface and driver shell script
# install (PROGRAMS gmt.m gmtmex.${MEX_EXT}
install (PROGRAMS gmt.m
	DESTINATION ${GMT_BINDIR}
	COMPONENT Runtime)

Apart from having to list ../gmt.h and ../gmt_version.h in gmtmex.h I got it to compile but fails on link because not looking for supplements.so. I guess I need to add that somehow?

FAILED: src/gmtmex/gmtmex.mexmaci64 
: && /Applications/OldXcode/11.7/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -std=gnu99  -ggdb3 -isysroot /Applications/OldXcode/11.7/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names  src/gmtmex/CMakeFiles/gmtmex.dir/gmtmex.c.o -o src/gmtmex/gmtmex.mexmaci64  -Wl,-rpath,/Applications/MATLAB_R2019a.app/bin/maci64  src/libgmt.6.2.0.dylib  /opt/local/lib/libnetcdf.dylib  /Applications/MATLAB_R2019a.app/bin/maci64/libmex.dylib  /Applications/MATLAB_R2019a.app/bin/maci64/libmx.dylib  /opt/local/lib/libcurl.dylib  /opt/local/lib/libgdal.dylib  /opt/local/lib/libgeos_c.dylib  /opt/local/lib/libpcre.dylib  /opt/local/lib/libfftw3f.dylib  /opt/local/lib/libfftw3f_threads.dylib  /opt/local/lib/libopenblas.dylib  -lm  -ldl  /opt/local/lib/libopenblas.dylib  -lm  -ldl  -framework  Accelerate  src/libpostscriptlight.6.2.0.dylib  /opt/local/lib/libz.dylib && :
Undefined symbols for architecture x86_64:
  "_GMTMEX_Get_Object", referenced from:
      _mexFunction in gmtmex.c.o

@seisman
Copy link
Member

seisman commented Sep 22, 2020

Try

target_link_libraries (gmtmex gmtlib ${GMT_SUPPL_LIB_NAME})

@PaulWessel
Copy link
Member Author

OK, just saw your post. I tried adding the supplements.so [${_suppl_lib_name}] to the target_link_libraries gave me more strangeness during the building of the executable. Perhaps

[269/270] Linking C executable src/gmtmex/gmtmex.mexmaci64
FAILED: src/gmtmex/gmtmex.mexmaci64 
: && /Applications/OldXcode/11.7/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -std=gnu99  -ggdb3 -isysroot /Applications/OldXcode/11.7/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names  src/gmtmex/CMakeFiles/gmtmex.dir/gmtmex.c.o -o src/gmtmex/gmtmex.mexmaci64  -Wl,-rpath,/Applications/MATLAB_R2019a.app/bin/maci64  src/plugins/supplements.so  src/libgmt.6.2.0.dylib  /opt/local/lib/libnetcdf.dylib  /Applications/MATLAB_R2019a.app/bin/maci64/libmex.dylib  /Applications/MATLAB_R2019a.app/bin/maci64/libmx.dylib  /opt/local/lib/libcurl.dylib  /opt/local/lib/libgdal.dylib  /opt/local/lib/libgeos_c.dylib  /opt/local/lib/libpcre.dylib  /opt/local/lib/libfftw3f.dylib  /opt/local/lib/libfftw3f_threads.dylib  /opt/local/lib/libopenblas.dylib  -lm  -ldl  /opt/local/lib/libopenblas.dylib  -lm  -ldl  -framework  Accelerate  src/libpostscriptlight.6.2.0.dylib  /opt/local/lib/libz.dylib && :
ld: can't link with bundle (MH_BUNDLE) only dylibs (MH_DYLIB) file 'src/plugins/supplements.so' for architecture x86_64

Now have a zoom in 3 minutes...will let you know if your arg makes a difference though.

@PaulWessel
Copy link
Member Author

No, same message as me. Wonder if those glues should go in libgmt.

@PaulWessel
Copy link
Member Author

Not yet, gotta do zoom.

@PaulWessel
Copy link
Member Author

Think I need to get gmtmex_parser.c into libgmt. That is where other glue is. I tried

set (GMT_LIB_SRCS ${GMT_LIB_SRCS} gmtmex_parser.c)

but does not stick.

@PaulWessel
Copy link
Member Author

Hi @joa-quim and @seisman, it seems to me that since we are adding the glue for some supplements (e.g., MB) into libgmt, we should do the same for the gmtmex part. So when we (or any enterprising user) build the bundle, it will have that mex-glue in it, and we should be able to build the gmtmex application similar to building the gmt application (different extension).

@seisman
Copy link
Member

seisman commented Sep 22, 2020

we are adding the glue for some supplements (e.g., MB) into libgmt.

Just curious, why do we have to include the MB-system glue in the GMT source codes? Should it be in the MB-system repository?

@PaulWessel
Copy link
Member Author

I think we have not really provided any of this (yet?) since, apart from @joa-quim on Windows, we do not supply any mb supplement through GMT. So perhaps it is latent code, but I see

set (GMT_MBSYSTEM_SRCS gmt_mbsystem_glue.c)

being added to libgmt. Need to get back to that doc page on supplements to relearn all this stuff. Quickly forget these things. But to answer you actual question: Yes, the idea was certainly for non-GMT supplements (like mb) to use gmt for this:

gmt --new-glue=mbsystem

and create their glue so that they can hook into GMT if they build completely separate from us. Largely undocumented on our side still.

With regards to gmtmex, I think it is slightly different in that we are providing two executables into GMT: One for command line (gmt) and one for MATLAB (gmtmex.mexmaci64) and possibly one for Octave as well. The octave one is perhaps more interesting in that it is open source and has no restrictions. It is just that I have Matlab so more likely to use it first. So in that sense these executables are not supplements and should be in src/CMakelists.txt and included/built if enabled by the Advanced settings.

@joa-quim
Copy link
Member

The gmtmex.mexmaci64|mex64 whatever are not executables. They are shared libs like .dll, .dylib, .so

Yes, we can include the mex_parser.c in our gmt.dll and then any user with Matlab, Octave and a compiler should be able to build the gmtmex.dll with a Matlab command. (-mex something)

But like always and don't like to follow the sheep and refuse to accept the list of Matworks sanctioned compiles and link directly to the Ml libs. This allows me for example to compile Mirone with the latest compiles that link with > 15 years old libs. The thing is that I want to maintain this capability to link directly to the Ml libs from the cmake recipe. Currently I use a batch script (in the gmtmex repo) to do that.

@PaulWessel
Copy link
Member Author

I figured you have control over the Win side so you would continue to build as you do. Our problems lie mostly on macOS and Linux side, but I think an octave option would be nice for Linux folks at least.

@PaulWessel
Copy link
Member Author

I think we had some old MB glue from before we completely refreshed this for 6.1 when we looked at automating and simplifying, so we probably kept that in there for backwards compatibility to help MB.

@PaulWessel
Copy link
Member Author

If I understand @joa-quim he advocates that

We include the mex_parser stuff in libgmt
We let users build their own gmtmex.* on their system since it is MATLAB version sensitive.

I think this sounds good. My main concern is the ability to debug the mex so I need to build gmtmex.mexmaci64 here. @joa-quim, I think there are more lines in gmtmex.c (at least toward the end) that we could call another function to make gmtmex.c as small as it needs to be (and put as much as we can in gmtmex_parser.c).

As fro octave, it is more like gs and gm and we could add the full shit in the macOS bundle, perhaps we should follow the same principle for MATLAB and just give clear instructions on how to build gmtmex.oct.

@PaulWessel
Copy link
Member Author

Sorry all these posts, but does this mean we keep the gmtmex separate repo but move gmtmex_parser.c and gmtmex.h to GMT main src then? And hence no supplement dir gmtmex since it is not going into that part?

@joa-quim
Copy link
Member

joa-quim commented Sep 22, 2020

let users build their own gmtmex.* on their system since it is MATLAB version sensitive.

I know that they have introduced some new things in the C mex API but I still link against R2009 libs and that has worked up to now.

If we move the the gmtmex_parser.c to the lib better move also the rest of the files to a gmtmex supplement. Moving some code from gmtmex.c to gmtmex_parser.c is possible but what difference would it make?

@PaulWessel
Copy link
Member Author

Just making the code the user has to deal with version-free (no checks for GMT version) and in general being smaller and easier to maintain, with out of the glue in libgmt instead. gmtmex.c is 400+ lines. I think many of the steps we do there is a bit too low-level. We did similar cleanup for the more C-like supplements (the new glue stuff).

OK, I can see moving gmtmex.c and instructions to a supplement dir with the blue in the libgmt core, but what about testing, docs, etc.? Do we imagine getting rid of the gmtmex repo entirely?

@joa-quim
Copy link
Member

If we move gmtmex.c then we should move everything, which is not that much btw.

@PaulWessel
Copy link
Member Author

I agree, and no tears over loosing old and wrong git history I think.

@joa-quim
Copy link
Member

No losses. The repo will continue to exist.

@PaulWessel
Copy link
Member Author

Before taking more action on this, let's review. My initial urge here was the (hopeful) belief that more of the gmtmex*.[ch] could go into the GMT core as glue. But clearly, all that code depends on MEX structures so there isn't much if anything to port. We can of course move gmtmex into GMT as a supplemental dir but it is different from all the others in that it would not build a supplement but a mex file, which is a shared lib/interface that can be run on the Matlab command line (and we do so via gmt.m). It would not go into supplements.so or anything since it is more like an executable in usage. Here are what I think are the reasons for doing this move and I hope @joa-quim can shoot these down if they are wrong:

  1. There is still almost 2000 lines of C code required to do gmtmex, and while a lot of that is calling mex functions, they are also calling GMT API functions. Unlike supplements, they are not using gmt_dev.h, only the API (gmt.h), so it is unlikely to need lots of GMT changes, but if they are needed then we are in the same repo.
  2. For us or a user to build GMT/MEX on macOS or potentially Linux, I assume it would be lots easier to do within GMT's cmake setup than having to maintain a separate build system (currently autoconf) for the gmtmex repo. In particular, I am concerned about making it easy for developers to debug. I think this was my main reason to try this.
  3. Related, it is hoped that building it together may reduce or eliminate the library conflicts we have with the Matlab applications which ships with older versions of libraries that we use.

The main problem with the GMT/MEX toolbox right now is that it is readily available on Windows, marginally available at times via a tarball build from me, and nothing on Linux. All hail Python and Julia, but we have lots of GMT core users funded by NSF who will continue to use Matlab, and we ought to make this a simpler experience.

@joa-quim
Copy link
Member

I don't disagree on the move. In fact, having it close to the GMT core makes it more visible to people ... that looks into the code. If there is a possibility to build it with our cmake recipes, better still.

However, since this Julia issue, I stated to doubt that the problem is with Matlab. It looks more a MacOS deep deep shit that we keep failing to understand.

@stale
Copy link

stale bot commented Dec 24, 2020

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@stale stale bot added the stale This will not be worked on label Dec 24, 2020
@stale stale bot closed this as completed Dec 31, 2020
@seisman seisman reopened this Dec 31, 2020
@stale stale bot closed this as completed Jan 10, 2021
@seisman seisman reopened this Jan 10, 2021
@stale stale bot removed the stale This will not be worked on label Jan 10, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@stale stale bot added the stale This will not be worked on label Jun 2, 2021
@stale stale bot closed this as completed Jun 16, 2021
@seisman seisman reopened this Jun 17, 2021
@stale stale bot removed the stale This will not be worked on label Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants