Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.89%. Comparing base (635cb9f) to head (915b7ce).
⚠️ Report is 3 commits behind head on main.

@@           Coverage Diff           @@
##             main     #221   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          24       24           
  Lines        4690     4690           
  Branches      878      878           
=======================================
  Hits         4685     4685           
  Partials        5        5           

It's OK to drop solsys2.$(SOEXT) from the Mac OS build. It's another legacy module that needs a custom fortran adapter to work. If someone really wants that, they can work it out how to build it themselves together with their own fortran code...

Ok! I'll get back at this after lunch haha

Looks pretty promising so far. Nice quick work! I'm impressed. ;-)

By the way, I went into the config and explicitly set the BUILTIN_SOLSYS2 ?= 0. I think the likes of that ifneq ($(BUILTIN_SOLSYS2),1) won't work properly if those variables are undefined because in make "" != 1is true. Why the linux build isn't trying to make the solsys2.so I have yet to figure out.

I suggest to put l.65 in the Makefile:

  SHARED_TARGETS += $(LIB)/libsolsys2.$(SOEXT)

inside a nested conditional, probably for Darwin (because I think it another Mac OS quirk). E.g.:

ifneq ($(BUILTIN_SOLSYS2),1)
  SOLSYS_TARGETS += $(OBJ)/solsys2.o
  # Don't build solsys2 shared lib on Mac OS -- it does not like mystery symbols.
  ifneq ($(shell uname -s),Darwin)
    SHARED_TARGETS += $(LIB)/libsolsys2.$(SOEXT)
  endif
endif

By the way, I went into the config and explicitly set the BUILTIN_SOLSYS2 ?= 0. I think the likes of that ifneq ($(BUILTIN_SOLSYS2),1) won't work properly if those variables are undefined because in make "" != 1is true. Why the linux build isn't trying to make the solsys2.so I have yet to figure out.

The point is that when solsys2 is not built-in then it is provided separately, as a .o object and a libsolsys2.so library -- either which can be linked against on demand, as needed. Built-in means that it is an integral part of libsupernovas.so. We only want that for solsys3 and not the others...

It looks like that for the Mac OS build you don't want to deal with solsys2 either way (neither built in, not shared lib), although the .o object could still be built... I also checked clang on Linux, and it does not bark. So it's another Mac OS quirk. I'll revise the above suggestion accordingly...

Makes sense! Boy do I love platform-specific quirks.

sigh mac's sed is not the same sed.

sigh mac's sed is not the same sed.

of course....

Oops I broke in install ci

It looks like the libsolsys*.$(SOEXT).$(SO_VERSION) build needs an explicit LD_LIBRARY_PATH (or DYLD_LIBRARY_PATH for MacOS) pointing to the lib/ directory... (so it can link against the so, before it's installed in the standard location after the build).

I think you want to add that to Makefile at:

$(LIB)/libsolsys%.so.$(SO_VERSION): | $(LIB) $(LIB)/libsupernovas.so
	$(CC) -o $@ $(SOFLAGS) -L$(LIB) -lsupernovas $(SHLIBS)

You probaly need an ifeq / else / endif to set it differently for Darwin like in make benchmark...

Alright @attipaci give this a look. I'm not super satisfied with checking the platform in three places, but I couldn't figure out a pattern to make it work right once.

I'm drafting up a CMake alternative too - just to sus it out a bit.

Thanks for all the effort. I'll look at it tomorrow, and see if I have any further ideas for improvement...

Alright I've cleaned up the logic quite a bit and consolidated the platform-specifics in one place in config.mk.

It turns out that this broke the shared libs build for Linux packaging. A fix is at PR #226. Generally, one should not set rpath when building the shared lib itself, especially not to a local directory that is not the final install location of the shared library. Instead, if you want to link a local shared library, you need to use the -L flag to tell the linker where (else) to look for shared libraries outside of the usual locations.

rpath probably need not be set in CMakeLists.txt either. For example, CALCEPH does not set CMAKE_INSTALL_RPATH... (PR #227)