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

[libtool,libltdl] New port #21731

Closed
wants to merge 23 commits into from
Closed

[libtool,libltdl] New port #21731

wants to merge 23 commits into from

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Nov 28, 2021

  • What does your PR fix?

    Adds a port for GNU libtool, to serve as a hook for performance and platform support improvements.
    The PR is now based on release 2.4.7. which fixes a performance regression, which is likely to affect windows even harder than other platforms, but is still present in msys2.
    The PR includes extra patches from latest msys2 libtool. AFAICT these patch should be compatible with other target triplets.

    For now, the updated libtool must be used explicitly, via a host dependency, or via a maintainer function.

    In addition to the libtool port (host dependency), this PR now includes a libltdl port (target dependency). This mirrors the packaging in MSYS2.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all, no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes

@dg0yt dg0yt force-pushed the libtool branch 2 times, most recently from a57e545 to 67b7776 Compare November 28, 2021 12:34
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 28, 2021

Elapsed time for package libtasn1:x64-windows in CI,
before this PR: 2.971 min, 2.645 min, 2.923 min,
with this PR: 2.366 min.
(Still a lot in contrast to linux, but a large share goes to autoreconf and configure.)

@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 29, 2021
@dg0yt dg0yt marked this pull request as ready for review November 29, 2021 06:17
@Neumann-A
Copy link
Contributor

probably better:

new port:
vcpkg-autotools <- make this the main dependency for all autotools based ports and make vcpkg-libtool a dependency of vcpkg-autotools and chain it into the autotools config (like vcpkg-qmake does with vcpkg-cmake)

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 29, 2021

probably better:

new port: vcpkg-autotools <- make this the main dependency for all autotools based ports and make vcpkg-libtool a dependency of vcpkg-autotools and chain it into the autotools config (like vcpkg-qmake does with vcpkg-cmake)

Makes sense. I could add it here as an empty port, version 0.

@Neumann-A
Copy link
Contributor

Makes sense. I could add it here as an empty port, version 0.

version-date instead of setting version to 0 ?

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 29, 2021

Makes sense. I could add it here as an empty port, version 0.

version-date instead of setting version to 0 ?

version-date is probaly okay for a vcpkg-autotools meta port which is not a physical autotools x.y.z port.

@JackBoosY
Copy link
Contributor

JackBoosY commented Nov 30, 2021

probably better:

new port: vcpkg-autotools <- make this the main dependency for all autotools based ports and make vcpkg-libtool a dependency of vcpkg-autotools and chain it into the autotools config (like vcpkg-qmake does with vcpkg-cmake)

If libtool takes too long to build, is it not worth it?
I very much hope that all the basic libraries and basic tools used in Linux are built based on the ports in vcpkg, but this should be considered.

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 30, 2021

If libtool takes too long to build, is it not worth it?

Libtool is build once, but used often.
Libtool wraps every call to the compiler and linker. In particular on windows, every fork or tool invocation hurts performance. So the extra build time for port libtool probably pays off.
(vcpkg might even add custom optimizations via patches.)

This PR started from an investigation how to improve performance for building with autotools on windows. While I didn't find a ready-to-use drop-in replacement for libtool, I found the sed regression patch which is not in msys.

@dg0yt dg0yt marked this pull request as draft June 12, 2022 12:06
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 12, 2022

libtool 2.4.7 was released in March. Time for a new attempt?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/gettext/vcpkg.json
  • ports/libiconv/vcpkg.json
  • ports/libtasn1/vcpkg.json

Valid values for the license field can be found in the documentation

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/gettext/vcpkg.json
  • ports/libiconv/vcpkg.json
  • ports/libtasn1/vcpkg.json

Valid values for the license field can be found in the documentation

@dg0yt dg0yt changed the title [libtool] New port [libtool,libltdl] New port Jun 13, 2022
@dg0yt dg0yt marked this pull request as ready for review June 13, 2022 05:30
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 13, 2022

Maybe the libtool port should be called vcpkg-tool-libtool now.

@JackBoosY
Copy link
Contributor

Can you please merged to master first?

@JackBoosY
Copy link
Contributor

Convert this PR to draft since there is no progress.

@JackBoosY JackBoosY marked this pull request as draft July 15, 2022 05:35
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 15, 2022

Note that this is blocked mostly be unclear position of vcpkg maintainers.

@BillyONeal
Copy link
Member

The team (@ras0219 / @ras0219-msft, @vicroms, @JavierMatosD, and I) looked at this in person in our team meeting today, 2022-08-04

  • We don't want vcpkg ports to be an application deployment system.
  • We don't believe a 20% or so performance improvement in downstream ports (which seems to be the primary reason to do this) warrants maintaining a custom built version of libtool.
  • We are concerned about the licensing and maintenance implications of this amount of patching in our repo; I understand that these are actually MinGW contributions and that you are not their author?
    • For example, we did something like this when we had no other choice with vcpkg-tool-ninja, but there we download the relevant patches from the upstream PR submission rather than incurring license concerns by merging them into our MIT licensed catalog.
  • We would be in favor of enabling you to use a different libtool by modernizing autoconf helpers, replacing vcpkg_configure_make / vcpkg_build_make and friends,port (in the same style as vcpkg-cmake and vcpkg-cmake-config).

@dg0yt dg0yt closed this Aug 5, 2022
@vividos
Copy link
Contributor

vividos commented Oct 12, 2022

@dg0yt would it be worthwile to at least add the ltdl port? It could be useful for a future port of libgphoto2, which uses ltdl as dependency...

@dg0yt dg0yt mentioned this pull request Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:documentation To resolve the issue, documentation will need to be updated category:new-port The issue is requesting a new library to be added; consider making a PR! requires:discussion requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants