ICU-10464 Make installation of icu-config optional #13

hughmcmaster · 2018-07-19T12:34:07Z

Make installation of icu-config optional.

Users who do not want to install icu-config must pass either --disable-icu-config or --enable-icu-config=no to configure.

From version 64.1, icu-config will not be installed by default.

Signed-off-by: Hugh McMaster hugh.mcmaster@outlook.com

CLAassistant · 2018-07-19T12:34:14Z

All committers have signed the CLA.

srl295 · 2018-07-19T14:43:08Z

icu4c/source/Makefile.in

@@ -169,7 +171,11 @@ all-local: $(srcdir)/configure $(LOCAL_BUILT_FILES) $(INSTALLED_BUILT_FILES)
 ifndef VERBOSE
 	@echo "Note: rebuild with \"$(MAKE) VERBOSE=1 $(MAKECMDGOALS)\" to show all compiler parameters."
 endif
+ifeq ($(INSTALL_ICU_CONFIG),true)
 install-local: install-icu install-manx


Minor, I see what this is trying to do, but could it instead conditionalize the line 69:

ifeq ($(INSTALL_ICU_CONFIG),true) MANX_FILES += config/icu-config.$(SECTION) endif

then manx-files becomes a no-op but the mechanism is still there if other man files show up (it could happen…)

Fine by me.

Actually, this doesn't work. make install fails because /usr/bin/install expects a file as an argument. We can solve this by checking whether MANX_FILES is set.

ifneq ($(MANX_FILES),) $(INSTALL_DATA) $? $(DESTDIR)$(mandir)/man$(SECTION) endif

Is this acceptable?

@hughmcmaster sounds good. thanks.

srl295 · 2018-07-19T14:45:18Z

icu4c/source/Makefile.in

@@ -187,7 +193,9 @@ install-icu: $(INSTALLED_BUILT_FILES)
 	@$(MKINSTALLDIRS) $(DESTDIR)$(libdir)/pkgconfig
 	$(INSTALL_DATA) $(ALL_PKGCONFIG_FILES) $(DESTDIR)$(libdir)/pkgconfig/
 	$(INSTALL_DATA) $(top_srcdir)/../LICENSE $(DESTDIR)$(pkgdatadir)/LICENSE
+ifeq ($(INSTALL_ICU_CONFIG),true)


👍 this is a good way to conditionalize, because we need icu-config to stay buildable.

srl295 · 2018-07-19T14:46:56Z

icu4c/source/configure.ac

@@ -100,6 +100,16 @@ UCONFIG_CPPFLAGS=""
 # such as -std
 UCONFIG_CFLAGS=""

+# Check whether to install icu-config


sadly, our configure script actually gets checked into the repo. (Maybe we should change that…) Can you run autoreconf || (aclocal && autoconf) ?

No problem.

hughmcmaster · 2018-07-21T08:17:47Z

@srl295 I've pushed the requested changes to this PR.

srl295

otherwise LGTM - but think we should default this to enabled for compatibility (at least for one release)

srl295 · 2018-07-23T21:21:05Z

icu4c/source/configure.ac

+            yes) enable_icu_config=true ;;
+            no) enable_icu_config=false ;;
+            *) AC_MSG_ERROR([bad value '${enableval}' for --enable-icu-config]) ;;
+        esac], [enable_icu_config=false])


sorry, one thing- can we make this default to true for now and announce that it will be false in the future?

Not a problem. If I change the default case to true, icu-config will be installed by default and users won’t need to specify any configure flags for this.

srl295 · 2018-07-23T22:45:03Z

@hughmcmaster please note that we are still defining some parts of our process. In particular, the JIRA ticket needs to be committee accepted before this merged. Thanks for your patience- you are one of the first few contributors via PR!

Signed-off-by: Hugh McMaster <hugh.mcmaster@outlook.com>

hughmcmaster · 2018-07-24T13:35:31Z

@srl295 I've made that change. Users must now pass either --enable-icu-config=no or --disable-icu-config to configure to prevent icu-config from being installed. If no icu-config-related flags are passed to configure, the script is installed by default.

srl295

LGTM. We will want to mention this in the release notes, and turn this off later.

srl295 · 2018-07-24T22:52:58Z

going to land this under the accepted ICU-10464 instead of ICU-20030

Signed-off-by: Hugh McMaster <hugh.mcmaster@outlook.com> Originally ICU-20030

ICU-20920 Add new MeasureUnit functions to units-staging

Hook up the data into units router

Missed updating the conan version number. This resulted in 69 reporting as 68. This is resolved via this patch.

# This is the 1st commit message: ICU-22767 Fix GCC warning and turn warning to errors # This is the commit message #2: ICU-22716 use pre-existing task # This is the commit message #3: ICU-22716 Fix # This is the commit message #4: ICU-22716 Fix unitialization # This is the commit message unicode-org#5: Update icu4c/source/common/ushape.cpp Co-authored-by: Fredrik Roubert <fredrik@roubert.name> # This is the commit message unicode-org#6: ICU-22716 change macro # This is the commit message unicode-org#7: ICU-22716 Add document about the macro # This is the commit message unicode-org#8: ICU-22716 Addres review feedback # This is the commit message unicode-org#9: ICU-22767 Fix GCC warning and turn warning to errors # This is the commit message unicode-org#10: ICU-22716 use pre-existing task # This is the commit message unicode-org#11: ICU-22716 Fix # This is the commit message unicode-org#12: ICU-22716 Fix unitialization # This is the commit message unicode-org#13: Update icu4c/source/common/ushape.cpp Co-authored-by: Fredrik Roubert <fredrik@roubert.name>

srl295 requested changes Jul 19, 2018

View reviewed changes

srl295 added the enhancement label Jul 19, 2018

hughmcmaster force-pushed the master branch 2 times, most recently from c7e221d to 9a80f40 Compare July 21, 2018 07:57

hughmcmaster changed the title ~~ICU-20030 icu4c: Make installation of icu-config optional~~ ICU-20030 Make installation of icu-config optional Jul 21, 2018

srl295 requested changes Jul 23, 2018

View reviewed changes

ICU-20030 Make installation of icu-config optional

7512823

Signed-off-by: Hugh McMaster <hugh.mcmaster@outlook.com>

hughmcmaster force-pushed the master branch from 9a80f40 to 7512823 Compare July 24, 2018 13:25

srl295 approved these changes Jul 24, 2018

View reviewed changes

srl295 changed the title ~~ICU-20030 Make installation of icu-config optional~~ ICU-10464 Make installation of icu-config optional Jul 24, 2018

srl295 self-assigned this Jul 24, 2018

srl295 merged commit 76168da into unicode-org:master Jul 24, 2018

sffc referenced this pull request in sffc/icu Sep 26, 2018

ICU-10464 Make installation of icu-config optional (#13)

64250f2

Signed-off-by: Hugh McMaster <hugh.mcmaster@outlook.com> Originally ICU-20030

sffc referenced this pull request in sffc/icu Sep 27, 2018

ICU-10464 Make installation of icu-config optional (#13)

66be632

Signed-off-by: Hugh McMaster <hugh.mcmaster@outlook.com> Originally ICU-20030

sffc referenced this pull request in sffc/icu Sep 27, 2018

ICU-10464 Make installation of icu-config optional (#13)

e5782e3

Signed-off-by: Hugh McMaster <hugh.mcmaster@outlook.com> Originally ICU-20030

sffc referenced this pull request in sffc/icu Sep 27, 2018

ICU-10464 Make installation of icu-config optional (#13)

e21982c

Signed-off-by: Hugh McMaster <hugh.mcmaster@outlook.com> Originally ICU-20030

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Jan 20, 2020

Merge pull request unicode-org#13 from sffc/unit-id

b76e3a1

ICU-20920 Add new MeasureUnit functions to units-staging

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Mar 25, 2020

Merge pull request unicode-org#13 from younies/units_router_younies

8d6d363

Hook up the data into units router

GerHobbelt pushed a commit to GerHobbelt/icu that referenced this pull request Mar 11, 2024

Merge pull request unicode-org#13 Update reported version number

753a012

Missed updating the conan version number. This resulted in 69 reporting as 68. This is resolved via this patch.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-10464 Make installation of icu-config optional #13

ICU-10464 Make installation of icu-config optional #13

hughmcmaster commented Jul 19, 2018 •

edited

Loading

CLAassistant commented Jul 19, 2018 •

edited

Loading

srl295 Jul 19, 2018

hughmcmaster Jul 20, 2018

hughmcmaster Jul 20, 2018

srl295 Jul 21, 2018

srl295 Jul 19, 2018

srl295 Jul 19, 2018

hughmcmaster Jul 20, 2018

hughmcmaster commented Jul 21, 2018

srl295 left a comment

srl295 Jul 23, 2018

hughmcmaster Jul 24, 2018

srl295 commented Jul 23, 2018

hughmcmaster commented Jul 24, 2018

srl295 left a comment

srl295 commented Jul 24, 2018

ICU-10464 Make installation of icu-config optional #13

ICU-10464 Make installation of icu-config optional #13

Conversation

hughmcmaster commented Jul 19, 2018 • edited Loading

CLAassistant commented Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughmcmaster commented Jul 21, 2018

srl295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Jul 23, 2018

hughmcmaster commented Jul 24, 2018

srl295 left a comment

Choose a reason for hiding this comment

srl295 commented Jul 24, 2018

hughmcmaster commented Jul 19, 2018 •

edited

Loading

CLAassistant commented Jul 19, 2018 •

edited

Loading