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

build: lib target now builds a shared library. #662

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Apteryks
Copy link

@Apteryks Apteryks commented Oct 7, 2024

A new 'LIBRARY' variable also causes the libraries to be installed along the headers and a newly introduced pkg-config file.

  • Makefile (DEFAULT) [LIBRARY]: Add lib to it. (CFLAGS): Add -fPIC.
    (PKGCONF_DIR, LIBS, PKGCONF_FILE): New variables.
    (lib): Express target via LIBS; add $(PKGCONF_FILE). (install) [LIBRARY]: Conditionally add lib and pkgconf targets, as well as associated recipes.
    ($(LIBDIR)/libsameboy.so, PKGCONF_FILE, pkgconf): New targets. (.PHONY): Register pkgconf.
  • README.md: Update doc.
  • sameboy.pc.in: New file.

@Apteryks
Copy link
Author

Apteryks commented Oct 7, 2024

I've used this successfully to build bsnes-jg (with a PR of its own to support that coming).

@Apteryks
Copy link
Author

Apteryks commented Oct 7, 2024

See https://gitlab.com/jgemu/bsnes/-/merge_requests/429 for an example of how this can be used.

sameboy.pc.in Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@orbea
Copy link
Contributor

orbea commented Oct 7, 2024

@LIJI32 Something this would require is caring about the API stability in the future and updating versioned libraries accordingly, I'm not sure your feelings on that.

Personally I would like better handling of the sameboy library, but only if you are also committed to the idea.

@LIJI32
Copy link
Owner

LIJI32 commented Oct 7, 2024

I've been gradually and intentionally stabilizing the API slowly the last few years, which is why libsameboy was introduced in the last major. The upcoming major release will finalize the API stabilization.

@Apteryks Apteryks force-pushed the build-and-install-shared-lib branch 3 times, most recently from 87f0c7b to 20ddb7f Compare October 10, 2024 14:26
@Apteryks
Copy link
Author

Apteryks commented Oct 10, 2024

Here's what the latest revision produces on my side:

$ make -j$(nproc) CC=gcc NATIVE_CC=gcc FREEDESKTOP=1 LIBRARY=shared install PREFIX=$PWD/install
[...]
$ find install/
install/
install/share
install/share/sameboy
install/share/sameboy/Shaders
install/share/sameboy/Shaders/AAOmniScaleLegacy.fsh
install/share/sameboy/Shaders/AAScale2x.fsh
install/share/sameboy/Shaders/AAScale4x.fsh
install/share/sameboy/Shaders/Bilinear.fsh
install/share/sameboy/Shaders/CRT.fsh
install/share/sameboy/Shaders/FlatCRT.fsh
install/share/sameboy/Shaders/HQ2x.fsh
install/share/sameboy/Shaders/LCD.fsh
install/share/sameboy/Shaders/MasterShader.fsh
install/share/sameboy/Shaders/MonoLCD.fsh
install/share/sameboy/Shaders/NearestNeighbor.fsh
install/share/sameboy/Shaders/OmniScale.fsh
install/share/sameboy/Shaders/OmniScaleLegacy.fsh
install/share/sameboy/Shaders/Scale2x.fsh
install/share/sameboy/Shaders/Scale4x.fsh
install/share/sameboy/Shaders/SmoothBilinear.fsh
install/share/sameboy/Palettes
install/share/sameboy/Palettes/Canyon.sbp
install/share/sameboy/Palettes/Desert.sbp
install/share/sameboy/Palettes/Evening.sbp
install/share/sameboy/Palettes/Fog.sbp
install/share/sameboy/Palettes/Green Slate.sbp
install/share/sameboy/Palettes/Green Tea.sbp
install/share/sameboy/Palettes/Lavender.sbp
install/share/sameboy/Palettes/Magic Eggplant.sbp
install/share/sameboy/Palettes/Mystic Blue.sbp
install/share/sameboy/Palettes/Pink Pop.sbp
install/share/sameboy/Palettes/Radioactive Pea.sbp
install/share/sameboy/Palettes/Rose.sbp
install/share/sameboy/Palettes/Seaweed.sbp
install/share/sameboy/Palettes/Twilight.sbp
install/share/sameboy/registers.sym
install/share/sameboy/background.bmp
install/share/sameboy/LICENSE
install/share/sameboy/dmg_boot.bin
install/share/sameboy/cgb_boot.bin
install/share/sameboy/sgb_boot.bin
install/share/sameboy/mgb_boot.bin
install/share/sameboy/cgb0_boot.bin
install/share/sameboy/agb_boot.bin
install/share/sameboy/sgb2_boot.bin
install/share/thumbnailers
install/share/thumbnailers/sameboy.thumbnailer
install/bin
install/bin/sameboy
install/bin/sameboy-thumbnailer
install/lib
install/lib/pkgconfig
install/lib/pkgconfig/sameboy.pc
install/lib/libsameboy.so.0.0.0
install/lib/libsameboy.so.0
install/lib/libsameboy.so
install/lib/libsameboy.la
install/include
install/include/sameboy
install/include/sameboy/apu.h
install/include/sameboy/camera.h
install/include/sameboy/cheat_search.h
install/include/sameboy/cheats.h
install/include/sameboy/debugger.h
install/include/sameboy/defs.h
install/include/sameboy/display.h
install/include/sameboy/gb.h
install/include/sameboy/joypad.h
install/include/sameboy/mbc.h
install/include/sameboy/memory.h
install/include/sameboy/model.h
install/include/sameboy/printer.h
install/include/sameboy/random.h
install/include/sameboy/rewind.h
install/include/sameboy/rumble.h
install/include/sameboy/save_state.h
install/include/sameboy/sgb.h
install/include/sameboy/sm83_cpu.h
install/include/sameboy/symbol_hash.h
install/include/sameboy/timing.h
install/include/sameboy/workboy.h

If I use LIBRARY=static, I get the .a instead of the .so, if I use LIBRARY=1, I get both.

When the 'LIBRARY' variable is set, the libraries are also now
installed along the headers and a newly introduced pkg-config file.

* Makefile (LT_CURRENT, LT_REVISION, LT_AGE, LT_VERSION_INFO): New
variables.
(DEFAULT) [LIBRARY]: Add lib to it.
(LT_MODE_ARG): New variable.
(strip_prefix, simplify_pkgconf_dir): New macros.
(prefix, exec_prefix, includedir, bindir, libdir, datadir)
(PKGCONF_EXEC_PREFIX, PKGCONF_INCLUDEDIR, PKGCONF_LIBDIR): New
variables.
(CFLAGS): Add -fPIC.
(PKGCONF_DIR, LIBTOOL_LIBRARY, PKGCONF_FILE): New variables.
(LIBTOOL, LIBTOOL_CC, LIBTOOL_LD): New variables.
(LIBFLAGS): Delete variable.
(CORE_LOBJECTS): New variable and target.
($(OBJ)/Core/%.c.o, $(OBJ)/SDL/%.c.o, $(OBJ)/XdgThumbnailer/%.c.o)
($(OBJ)/OpenDialog/%.c.o, $(OBJ)/%.c.o: %.c)
($(OBJ)/HexFiend/%.m.o: HexFiend/%.m)
($(OBJ)/%.m.o: %.m): Use $(LIBTOOL_CC).
(lib): Express target via LIB_STATIC and LIB_SHARED; add
$(PKGCONF_FILE).
(install_headers, install_pkgconf_file): New macros.
(install) [LIBRARY]: Conditionally add lib and pkgconf targets, as
well as associated recipes. Adjust installation paths to use new
variabes.
($(LIBDIR)/libsameboy.o, $(LIBDIR)/libsameboy.a): Delete targets.
($(LIBTOOL_LIBRARY), $(PKGCONF_FILE), pkgconf): New targets.
(.PHONY): Register pkgconf. Add missing space between lib/lib-unsupported
* README.md: Update doc.
* sameboy.pc.in: New file.
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
ifneq (, $(shell command -v libtool 2> $(NULL)))
LIBTOOL := libtool
LIBTOOL_CC := $(LIBTOOL) --tag=CC --mode=compile $(CC) -c
LIBTOOL_LD := $(LIBTOOL) --tag=CC --mode=link $(CC) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you really shouldn't be using libtool at all if you aren't use autotools, this is not going to be portable for slibtool.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems slibtool (which I had no idea existed until now) is not fully compatible with libtool; am I not sure I understand the compatibility argument. Is it not a fault of slibtool to not being fully compatible with libtool? Where would this be an issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gentoo allows using slibtool where it works with most packages.

https://wiki.gentoo.org/wiki/Slibtool

Using libtool outside of autotools is always problematic and an extremely suboptimal way to use libtool. The GNU libtool script is one of the worst things ever written (Which is why slibtool exists).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using libtool outside of autotools is always problematic and an extremely suboptimal way to use libtool.

Could you expound on why you say it's "always problematic" to use libtool outside of Autotools? I'm curious of the reasons. I've found some peculiarities a bit challenging while authoring this diff, but I don't foresee problems with it. If Gentoo prefers slibtool, it seems that should work should work using LIBTOOL=slibtool as well, assuming the basic features of libtool I've made used are covered by this alternative implementation of it.

Regarding the quality of libtool itself, I don't particularly mind how pretty or ugly the innards of its implementation are; it's a battle-tested solution that simplified the problem at hand.

I don't mind revising this series more, but I'll need specific problems to address (I don't count "libtool is ugly" as one). Thank you for your continued input!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GNU libtool is supposed to work by generating a ./libtool in the build tree during the ./configure process using LT_INIT from autoconf. The default slibtool symlink to use is rlibtool which depends on this existing so it can determine if it needs shared or static libraries and it will fail if this does not exist or if slibtoolize is not used instead.

Since this is not autotools there is ./configure, LT_INIT or libtoolize/slibtoolize and it will fail with flow errors.

https://wiki.gentoo.org/wiki/Slibtool#flow_error:_unexpected_condition_or_other

To be very blunt, this solution is entirely a non-starter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we s/libtool/slibtool/ and be done, if it fixes all these libtool shortcomings? Or is more complicated than than to switch to slibtool?

Description: The SameBoy library
Version: @version@
Cflags: -I${includedir}/sameboy
Libs: -L${libdir} -lsameboy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When linking libsameboy.a -lm appears to be needed

Libs.private: -lm

-@rm $(OBJ)/lto_hack.o

$(LIBDIR)/libsameboy.a: $(LIBDIR)/libsameboy.o
$(LIBTOOL_LD) -rpath $(libdir) $^ -o $@
Copy link
Contributor

@Jan200101 Jan200101 Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rpaths are a general cause for problems and are discouraged wherever possible.

Fedora outright bans a lot of rpaths it considers "broken"
https://fedoraproject.org/wiki/Changes/Broken_RPATH_will_fail_rpmbuild#Definition_of_a_broken_RPATH

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw GNU libtool breaks without the -rpath (While Slibtool doesn't have this problem). Another reason to avoid using libtool here...

includedir ?= $(prefix)/include
bindir ?= $(exec_prefix)/bin
libdir ?= $(exec_prefix)/lib
datadir ?= $(prefix)/share
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be affected by or affect DATA_DIR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that DATA_DIR is for that SameBoy specific data files like the bios while datadir is for package specific data files like the icons and desktop file. In autotools this is the difference between datadir and datarootdir.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I think the Makefile should have those two variables here as well.
Also, since a specific naming convention was already established with DATA_DIR and co., should these also follow the naming scheme?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Autotools convention is widely understood for packagers, so makes sense to follow. I was a bit confused that DATA_DIR meant something different than datadir for SameBoy at first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the autotools conventions you are using datadir as if it was datarootdir and DATA_DIR is what datadir would be.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I had missed that. Then I guess I should rename the datadir I used to datarootdir and have a new datadir be an alias to DATA_DIR.

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

Successfully merging this pull request may close these issues.

4 participants