astyle: support the system astyle library
ClosedPublic

Authored by pino on Dec 23 2018, 1:47 PM.

Details

Summary

Find the astyle library installed in the system, and use it instead of
the embedded copy (which is still used if the system library is not
available).

Test Plan

Builds fine with, and without a system libastyle (the embedded copy is used in the latter case).
test_astyle passes in both cases.
I did not try the plugin for real, though.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pino created this revision.Dec 23 2018, 1:47 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptDec 23 2018, 1:47 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
pino requested review of this revision.Dec 23 2018, 1:47 PM
pino updated this revision to Diff 48070.Dec 23 2018, 1:48 PM

Remove accidental changes

pino edited the test plan for this revision. (Show Details)Dec 23 2018, 1:54 PM
pino added a comment.Dec 23 2018, 2:19 PM

Side note: another approach can be to build the astyle plugin only if a system astyle library is found, i.e. make the plugin optional, and get rid of the astyle embedded copy.
I can adopt that approach -- right now I chose the most "conservative" approach.

Thanks for the patch, @pino, agreed that system things should be preferred (curious myself why there even is a copy).
No time to look in details currently, but IIRC there are some API changes between astyle versions, so version needs to be taken into account instead of just using whatever is provided by the system. Cmp. commit which upgraded the astyle copy recently: 378a6e02cec55390c6dc903d98a035485ec1aa10

pino added a comment.Dec 23 2018, 2:32 PM

No time to look in details currently, but IIRC there are some API changes between astyle versions, so version needs to be taken into account instead of just using whatever is provided by the system. Cmp. commit which upgraded the astyle copy recently: 378a6e02cec55390c6dc903d98a035485ec1aa10

My idea is to do feature checks at cmake time to adapt to the API changes. I don't have astyle older than 3.1 to test, though (I can help fixing these issues, of course),

mwolff requested changes to this revision.Jan 28 2019, 10:36 AM
mwolff added a subscriber: mwolff.

do we have custom patches in our libastyle? I hope not, but it's been too long for me to remember. If not, then I guess we should get rid of our copy of lbiastyle and just disable kdevastyle if that dependency wasn't found - it's pretty optional anyways.

if we do have custom patches, then I wonder if it's safe/fine to use the non-patched version? have you tried, did it work for the common stuff?

This revision now requires changes to proceed.Jan 28 2019, 10:36 AM
pino added a comment.Jan 28 2019, 8:28 PM

do we have custom patches in our libastyle?

It looks not, at least according to the changes in aecd24c2f384d500de1dc83f8ba3fb4f2fd8323d, and 378a6e02cec55390c6dc903d98a035485ec1aa10.

I hope not, but it's been too long for me to remember. If not, then I guess we should get rid of our copy of lbiastyle and just disable kdevastyle if that dependency wasn't found - it's pretty optional anyways.

I can do that (mentioned in a previous comment).

ok cool, then please get rid of our internal copy!

Question: does any distro package that?

At least my openSUSE doesn't nor any other distro I know of, only thing I found was:

https://rpmfind.net/linux/rpm2html/search.php?query=libastyle-devel

arrowd added a subscriber: arrowd.Mar 11 2019, 11:48 AM

Question: does any distro package that?

At least my openSUSE doesn't nor any other distro I know of, only thing I found was:

https://rpmfind.net/linux/rpm2html/search.php?query=libastyle-devel

I skimmed through https://repology.org/project/astyle/packages and found none.

mwolff accepted this revision.Mar 11 2019, 2:31 PM

ah, then let's get this in as-is

This revision is now accepted and ready to land.Mar 11 2019, 2:31 PM
kossebau requested changes to this revision.Mar 11 2019, 2:50 PM
kossebau added inline comments.
plugins/astyle/CMakeLists.txt
3

Please check for version 3.*, given the current code relies on its API (and config keys?).

Not exactly sure which 3.*, 378a6e02cec55390c6dc903d98a035485ec1aa10 tells which kdevelop-side API changes had be done there.

This revision now requires changes to proceed.Mar 11 2019, 2:50 PM
pino updated this revision to Diff 54059.Mar 17 2019, 8:47 AM
  • add some rudimental version detection of the astyle library, since it provides no pkg-config file nor version macros/variables...
  • request libastyle >= 3.1
pino marked an inline comment as done.Mar 17 2019, 8:47 AM
pino added a subscriber: lbeltrame.Mar 17 2019, 8:58 AM

Question: does any distro package that?

At least my openSUSE doesn't nor any other distro I know of, only thing I found was:

https://rpmfind.net/linux/rpm2html/search.php?query=libastyle-devel

I skimmed through https://repology.org/project/astyle/packages and found none.

  • on Debian (and thus Ubuntu and any other derivative) there's libastyle-dev
  • on Fedora (and thus any derivative) there's astyle-devel
  • on Mageia (and thus any derivative) there's libastyle-devel
  • IIRC the Gentoo ebuild builds the library as well

Can you please request openSUSE to package the astyle library? @lbeltrame can you help with this (even if it isn't your realm, you might know who could)?

Is this part of the astyle tarball, or a separate project?

pino added a comment.Mar 17 2019, 1:02 PM

Is this part of the astyle tarball, or a separate project?

Same source, it needs to be enabled passing the right variable to the makefile, or cmake (depending how astyle is built in openSUSE).

Then the best should be to file a bug on bugzilla.opensuse.org asking for that to be packaged.

apol accepted this revision.May 25 2019, 3:11 AM
kfunk accepted this revision.Mon, May 27, 6:36 AM
kfunk added a subscriber: kfunk.

Works fine for me on Ubuntu 19.04 -- lgtm!

This revision was not accepted when it landed; it landed in state Needs Review.Mon, May 27, 6:43 AM
This revision was automatically updated to reflect the committed changes.