Skip to content

ICU-5628 Fix using install-sh in out-of-source builds - #4109

Open
dg0yt wants to merge 1 commit into
unicode-org:mainfrom
dg0yt:install-sh
Open

ICU-5628 Fix using install-sh in out-of-source builds#4109
dg0yt wants to merge 1 commit into
unicode-org:mainfrom
dg0yt:install-sh

Conversation

@dg0yt

@dg0yt dg0yt commented Aug 7, 2026

Copy link
Copy Markdown

Cf. microsoft/vcpkg#53279:
icu's configure.ac uses AC_PROG_INSTALL which tries to find a fast binary install and uses a slow install-sh script as fallback. Via @INSTALL@, the result is baked into icudefs.mk in the toplevel build directory.
When the fallback is used, icu prepends $(top_srcdir) for in-source builds (i.e. ${srcdir} = "."). (I assume this was meant as a fix for ICU-525 which is the same error for in-source builds.)
For out-of-source builds, nothing is prepended, and so the value of INSTALL is valid only for the toplevel build dir. This leads to errors when it is actually used in subdirs.
This change prepends $(top_builddir) instead, so that a valid path is construct for every possible case. Prepending is scoped to situations where srcpath is relative.

To reproduce the original issue, rename /usr/bin/install and similar candidates before running ../path/to/icu/source/configure.

The change from this PR is in configure.ac. configure is generated from that file.

Checklist

  • Required: Issue filed: ICU-5628
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable
  • Approver: Feel free to merge on my behalf

@jira-pull-request-webhook

Copy link
Copy Markdown

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@dg0yt dg0yt changed the title [DRAFT] Fix using install-sh in out-of-source builds ICU-5628 Fix using install-sh in out-of-source builds Aug 7, 2026
Comment thread icu4c/source/configure.ac
INSTALL="\\\$(top_srcdir)/${ac_install_sh}"
fi
# If using the included install-sh, make INSTALL work from any build dir.
if test "${ac_install_sh}" = "${INSTALL}"; then

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Unchanged guard, and matching an effect in AC_PROG_INSTALL.

Comment thread icu4c/source/configure.ac
# If using the included install-sh, make INSTALL work from any build dir.
if test "${ac_install_sh}" = "${INSTALL}"; then
case "$srcdir" in
.|./*|../*) INSTALL="\\\$(top_builddir)/${ac_install_sh}" ;;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Guard based on srcdir, similar to original code. I believe srcdir makes sense because it is used to construct the search path - and thus becomes part of the value - for install-sh.
Now any relative path instead of just ., and now prepending top_builddir instead of top_srcdir.

@dg0yt
dg0yt marked this pull request as ready for review August 7, 2026 18:40
@dg0yt

dg0yt commented Aug 7, 2026

Copy link
Copy Markdown
Author

Disclaimer: I did not test this with in-source builds.

@dg0yt

dg0yt commented Aug 8, 2026

Copy link
Copy Markdown
Author

This fix is pointless with the 25 year old install-sh which is unable to handle multiple files, as required for

$(INSTALL_DATA) $(ALL_PKGCONFIG_FILES) $(DESTDIR)$(libdir)/pkgconfig/

$(INSTALL_DATA) $(docfiles) $(DESTDIR)$(docdir)/$(docsubdir)

Up-to-date version: https://github.com/autotools-mirror/autoconf/blob/master/build-aux/install-sh

@markusicu

Copy link
Copy Markdown
Member

Disclaimer: I did not test this with in-source builds.

Please do test.

This fix is pointless with the 25 year old install-sh which is unable to handle multiple files

So we should probably update that first, and test that that update does not break things.
And we will need to check if the license changed, and if so, if it's compatible with ICU.

@srl295 @roubert WDYT?

@markusicu markusicu added the incomplete Needs work; do not approve/merge as is. label Aug 13, 2026
@roubert

roubert commented Aug 13, 2026

Copy link
Copy Markdown
Member

Agreed, removing the use of install-sh seems like a very good idea but let's do this in the right order and verify at every step that nothing accidentally breaks.

@srl295

srl295 commented Aug 13, 2026

Copy link
Copy Markdown
Member

Looks good but agreed, needs to be tested at each step.

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

Labels

incomplete Needs work; do not approve/merge as is.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants