Skip to content

Add compatibility headers #420

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

Merged
merged 1 commit into from
Jul 10, 2016
Merged

Add compatibility headers #420

merged 1 commit into from
Jul 10, 2016

Conversation

AdamMajer
Copy link
Contributor

In older versions of tidy, these headers were defined. Now, they are
renamed to tidybuffio.h and tidyplatform.h. This may be more of a
consistent naming scheme, but it breaks current software.

Re-add these headers and add compile time warning when such a header
is used.

@geoffmcl
Copy link
Contributor

@AdamMajer thank you for the PR... always welcome...

Yes, this was a tough decision for our first release in about 7+ years... see #223 and others... you will note the alternative of installing in like <tidy/tidy.h> was also considered, and rejected, at that time... could be reconsidered for future releases...

So while there will remains a possible conflict with other generic header names, if installed in /usr/include or /usr/local/include, it seems a benign cmake option... It defaults to OFF, and only users with the knowledge, and should know what they are doing, would set it ON...

But after 7+ years I doubt there would be much current software out there using those old headers... but maybe...

Other major packages, that are likely to have links with libtidy, like say PHP, and hopefully others... already use the re-named headers from using later tidy sources, maybe due to no distribution updates, to build their own distribution libraries and/or other binaries... so these are aware...

All known windows apps that use lib tidy DLL never depend on a globally installed Tidy DLL! It does not exist, yet! Most have updated from source, so this is not needed in windows... And that is good because the #warning "ABC" gcc macro used is not MSVC compatible... it is an error!

Will leave it for a little time, for others to comment, but looks ok to me, at this time...

If there is a use case, why not? Comments welcome...

@AdamMajer
Copy link
Contributor Author

On Wed, 15 Jun 2016 18:08:10 -0700
Geoff McLane [email protected] wrote:

So while there will remains a possible conflict with other generic
header names, if installed in /usr/include or /usr/local/include,
it seems a benign cmake option... It defaults to OFF, and only
users with the knowledge, and should know what they are doing, would
set it ON...

But after 7+ years I doubt there would be much current software
out there using those old headers... but maybe...

I think apache2 tidy mod broke along with others that now build
with old tidy. Maybe because I've installed headers
under /usr/include/tidy not /usr/include? Would have to check on that.

All known windows apps that use lib tidy DLL never depend on a
globally installed Tidy DLL! It does not exist, yet! Most have
updated from source, so this is not needed in windows... And that
is good because the #warning "ABC" gcc macro used is not MSVC
compatible... it is an error!

That pragma could be commented with a #ifdef GNUC for example. And
have special one for MSVC so it doesn't break :) I think MSC_VER
should work.

When I still ship programs for Windows, I always use DLLs, although
locally installed ones (in program's install directory, not some
global directory or as SxS). For Linux distributions, globally
installed libtidy is very important for various reasons, but security
updates is most important. This is why static linking for Linux distros
is generally not allowed.

@geoffmcl
Copy link
Contributor

@AdamMajer yes, absolutely agree, even after 7+ years, there may be cases where compatible (old) headers may be required...

I think apache2 tidy mod broke along with others ...

If you are referring to - http://mod-tidy.sourceforge.net/ - yes, I note src/mod_tidy.c (2006) uses "buffio.h". But in the source I built in linux, and windows, had embedded an old tidy source, with the old headers, so does not use any installed tidy-dev headers... and in fact effectively uses tidy in a static like form, ie not using a shared library...

And also note that site has not been updated in quite a while, and still points people to our old historic dormant SF site... must add it to my TODO list to try to contact them...

But, like I say, there could be others, thus support this PR...

And yes there are various macros that can be used as a substitute for the gcc #warning "ABC"... But in Windows people who build tidy from source would have no need to enable this option... Seldom, probably never would a windows user actually 'install' tidy, at least not to the meaningless Program Files folders in tidy's case...

In development, I do install tidy into a <some-root>\3rdParty, so I can test the FindTidy.cmake module correctly finds things, but of course all the test sources there already use the modified headers, so would never need these compatibility headers... and if by accident was compiling some old third party package, that still used these headers, prefer the compile error, so it can be fixed... rather than a soft warning message... so what is there is ok...

Will merge this shortly...

@danielhjames
Copy link
Contributor

Arch Linux is working around this with symlinks: https://git.archlinux.org/svntogit/packages.git/commit/trunk?h=packages/tidy&id=482ab3f78fcd66828214eafeb6b2d66c11306e1c

I've had a request to do the same for the Debian package of version 5.2.0, does that seem like a reasonable short-term solution?

@AdamMajer
Copy link
Contributor Author

On 06/18/2016 07:19 PM, Geoff McLane wrote:

linux, and windows, had embedded an old tidy source, with the old
headers, so does not use any installed tidy-dev headers... and in fact
effectively uses tidy in a static like form, ie not using a shared
library...

Just to be clear 100%,

NONE of Linux distributions would use static linking (maybe except
the one that rebuilds everything anyway :)

ALL Linux distributions use dynamic linking.

It's been like that since the beginning. There are major problems with
things like Go (programming language) that does not support dynamic linking.

  • Adam

@oerdnj
Copy link

oerdnj commented Jun 28, 2016

But after 7+ years I doubt there would be much current software out there using those old headers

Out of this list (using codesearch.debian.org to lookup usages of buffio.h and filtered whether they build-depend on libtidy-dev):

libhtml-tidy-perl
psi-plus
edbrowse
xqilla
php5
kde-baseapps
elementtidy
libextractor
kdewebdev
abiword
pumpa
prayer
libopkele
php7.0

only edbrowse also has tidybuffio.h referenced in the source code.

So you broke 14 of packages that build-depend on libtidy-dev in Debian. That's 50%. Arch Linux already have compatibility symlinks in place. And Debian will have to follow just to avoid the headache of patching all those packages.

In older versions of tidy, these headers were defined. Now, they are
renamed to tidybuffio.h and tidyplatform.h. This may be more of a
consistent naming scheme, but it breaks current software.

Re-add these headers and add compile time warning when such a header
is used.
@AdamMajer
Copy link
Contributor Author

Updated header patch so that it will not cause errors on non-gnu (and non-clang) compilers, which basically means MSVC and niche compilers.

@geoffmcl geoffmcl merged commit 517a2ea into htacg:master Jul 10, 2016
geoffmcl added a commit that referenced this pull request Jul 10, 2016
@geoffmcl
Copy link
Contributor

While not totally happy! with this, do see a certain use case...

Anyway, now merged... thanks @AdamMajer

And version bumped to 5.3.6...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants