Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
build: lib target now builds a shared library. #662
Changes from all commits
d1c47a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 whiledatadir
is for package specific data files like the icons and desktop file. In autotools this is the difference betweendatadir
anddatarootdir
.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 wasdatarootdir
andDATA_DIR
is whatdatadir
would be.There was a problem hiding this comment.
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 todatarootdir
and have a newdatadir
be an alias toDATA_DIR
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
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 usingLT_INIT
fromautoconf
. The default slibtool symlink to use isrlibtool
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 ifslibtoolize
is not used instead.Since this is not autotools there is
./configure
,LT_INIT
orlibtoolize
/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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...There was a problem hiding this comment.
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