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

static linking by default? (on Linux) #326

Closed
vcunat opened this issue Nov 28, 2015 · 12 comments
Closed

static linking by default? (on Linux) #326

vcunat opened this issue Nov 28, 2015 · 12 comments
Milestone

Comments

@vcunat
Copy link

vcunat commented Nov 28, 2015

(Suggested label: Build/Install/Distribute)

Currently when I build it, bin/tidy is statically linked, and both lib/libtidy.so and lib/libtidys.a are installed. I believe it would be better to default to dynamic linking and not installing *.a, at least on unix-like platforms.

Side note: I'm creating a package for Nix / NixOS.

@vcunat
Copy link
Author

vcunat commented Nov 28, 2015

Ah, we already have a 4.9 package in there, separately from the old html-tidy. I assume this project is (also) meant as a drop-in replacement for the old html-tidy, including libtidy, right? From a quick skim I only see missing bin/tab2space, but perhaps that was rarely used.

@geoffmcl
Copy link
Contributor

@vcunat yes, at this time the tidy console binary is linked with the static library, mainly as a convenience to Windows users where we do not have a convenient DLL (shared library) install mechanism. Easier to have a stand-alone app...

And as far as I can see it poses no problem in linux. And in fact, as a developer, it is convenient for me having several stand-alone versions of tidy around for backward testing. But as things settle down we will consider changing this, for linux only, just to conform to the norm... but will continue to build and install both libraries, allowing 3rdParty libtidy or libtidys users a choice...

The tab2space is now an optional build, and even if built, is no longer included in the install or packages. This local only utility was just for developers that had not adjusted their favorite editor to conform to the tidy code style of using 4-space indents, and no tabs, so is seldom required, and there are other tools/utilities that can do the same job... probably better...

If you have a 4.9 package there already, I think this will be tidy5, and libtidy5.so, not sure? These should be removed... that suffix 5 was just used in development, when we were considering tidy in two forms - tidy for html4-- (old tidy), and tidy5 for html5++. We have since decided there should be only one tidy which handles all...

So, yes, 5.?.? is meant as a drop-in replacement for old tidy, removing that 5 suffix... it is good that you are updating the package for Nix / NixOS. We hope other distribution will also update. We have some problem with distro with even older versions available... see #309 and others...

Will leave this open for a while for further comments... if any... but see nothing at present to do here...

@geoffmcl geoffmcl added this to the 5.1 milestone Nov 28, 2015
@vcunat
Copy link
Author

vcunat commented Nov 28, 2015

Oh, is there no option to link bin/tidy dynamically? https://github.com/htacg/tidy-html5/blob/master/CMakeLists.txt#L189 We and most other Linux distributions tend to prefer dynamic linking. The old version is fine in this respect.

I plan to replace both versions by unified 5.*. It seems that reverse dependencies build fine, except for prayer-1.3.5 which gets broken: html_secure_tidy.c:15:20: fatal error: buffio.h: No such file or directory.

BTW, nix is specifically designed to be able to keep any number of versions and configurations around, even with all its dependency closures (dynamically linked and other references).

@geoffmcl
Copy link
Contributor

@vcunat ok seems I must read tend to prefer as must ;=))

With the beginning of the 5.* packages, two generically named header files were changed, to avoid any conflict with other package headers - see #223

  1. renamed: "buffio.h" -> "tidybuffio.h"
  2. renamed: "platform.h" -> "tidyplatform.h"

Will certainly explore adding a cmake option to link the tidy console app to the shared libray...

@vcunat
Copy link
Author

vcunat commented Nov 28, 2015

I see; if the headers only got renamed, the breakages will be fixed easily and safely.

vcunat added a commit to NixOS/nixpkgs that referenced this issue Nov 29, 2015
- the original project has been unmaintained for years
- some dependants needed to be patched due to renamed headers
  htacg/tidy-html5#326 (comment)
- separate tidy-html5 package was removed, as since the 5.0.0 version
  it's meant as a successor to both, and library name got back
  from libtidy5.so to libtidy.so
  htacg/tidy-html5#326 (comment)

/cc committers to tidy-html5: @edwjto and @zimbatm.
geoffmcl added a commit that referenced this issue Dec 4, 2015
@geoffmcl
Copy link
Contributor

geoffmcl commented Dec 4, 2015

@vcunat added a cmake option -DTIDY_CONSOLE_SHARED:BOOL=YES to link the console app tidy to the shared library, Of course the BUILD_SHARED_LIB must remain on.

Have tested all combinations in windows, and all got built/linked as expected.

Much appreciated if you could get a chance to pull 5.1.30++ and give it a try... thanks...

@vcunat
Copy link
Author

vcunat commented Dec 4, 2015

Looks good, except that during build it tries to run the executable and fails to find the library. I worked around that easily, though. The binary is tiny now.

@balthisar
Copy link
Member

I've been experimenting with localization and this seems like a good idea as binary size grows significantly with added languages. This would help mitigate that.

@vcunat
Copy link
Author

vcunat commented Dec 5, 2015

I thought it standard to have the translated texts stored in separate per-language files.

@balthisar
Copy link
Member

It would be preferable but compiling them in maintains universal architecture support without having to consider the platform specifics of every operating system in existence.

@vcunat
Copy link
Author

vcunat commented Dec 5, 2015

I wouldn't expect such a small project to program this from scratch. There are standard frameworks for handling translations in a cross-platform way.

@geoffmcl geoffmcl modified the milestones: 5.2, 5.1 Dec 11, 2015
@balthisar
Copy link
Member

With the Cmake fix and the discussion going off topic, I'll tidy up and close this issue.

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

No branches or pull requests

3 participants