Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

create full-icu locale tarballs #9091

Closed
wants to merge 2 commits into from
Closed

create full-icu locale tarballs #9091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
887d061
build: i18n: include icu conversion code in the 'host' toolset.
srl295 Jan 24, 2015
40b87cc
build: intl: add 'data-tarballs' target to generate .tar.gz of full I…
srl295 Jan 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,16 @@ BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
BINARYTAR=$(BINARYNAME).tar.gz
PKG=out/$(TARNAME).pkg
PACKAGEMAKER ?= /Developer/Applications/Utilities/PackageMaker.app/Contents/MacOS/PackageMaker
LEDATATAR=$(TARNAME)-locales-le.tar
BEDATATAR=$(TARNAME)-locales-be.tar
LEDATATGZ=$(LEDATATAR).gz
BEDATATGZ=$(BEDATATAR).gz
#ICU_DATA_IN=$(firstword $(wildcard deps/icu/source/data/in/icudt*l.dat))
Copy link

Choose a reason for hiding this comment

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

I would remove all comments from this PR if possible, otherwise it makes it difficult for reviewers to know if commented out code should be reviewed or not.

#ICU_DATA_SRC=out/Release/gen
ICU_DATA_SRC=deps/icu/source/data/in
Copy link

Choose a reason for hiding this comment

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

Do you think we could add a ICU_DATA_IN in the config.mk file that is generated from the configure script to avoid having it hardcoded in several different places?

ICU_DATA_IN=$(firstword $(wildcard $(ICU_DATA_SRC)/icudt*[bl].dat))
Copy link

Choose a reason for hiding this comment

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

@tjfontaine Do we generally allow using GNU make extensions in the Makefile?

Copy link

Choose a reason for hiding this comment

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

I think there are a lot of places already where we assume GNU make, fairly sure gyp will use gmake on the bsds and sunos based systems

ICU_DATA_BE=$(ICU_DATA_IN:$(ICU_DATA_SRC)/icudt%l.dat=icudt%b.dat)
ICU_DATA_LE=$(ICU_DATA_IN:$(ICU_DATA_SRC)/icudt%l.dat=icudt%l.dat)

PKGSRC=nodejs-$(DESTCPU)-$(RAWVER).tgz
ifdef NIGHTLY
Expand Down Expand Up @@ -441,3 +451,19 @@ cpplint:
lint: jslint cpplint

.PHONY: lint cpplint jslint bench clean docopen docclean doc dist distclean check uninstall install install-includes install-bin all staticlib dynamiclib test test-all test-addons build-addons website-upload pkg blog blogclean tar binary release-only bench-http-simple bench-idle bench-all bench bench-misc bench-array bench-buffer bench-net bench-http bench-fs bench-tls

## Build data tarballs
data-tarballs: $(LEDATATGZ) $(BEDATATGZ)

$(LEDATATGZ) $(BEDATATGZ): node
Copy link

Choose a reason for hiding this comment

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

Depending on how node has been built, icupkg may not have been built. Instead of depending on node, maybe we could make these two targets depend on icupkg? In the current state of the build system, an icupkg target would depend on calling ./configure --with-intl=small-icu first.

Copy link
Member Author

Choose a reason for hiding this comment

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

icupkg isn't available in some configurations. maybe better to have this code run configure&&make directly.

Copy link

Choose a reason for hiding this comment

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

But it is used in the rule that builds these targets, so it must be available or am I missing something?

# rm -rf $(BINARYNAME) out/deps out/Release
# $(PYTHON) ./configure --download=all --with-intl=full-icu
Copy link

Choose a reason for hiding this comment

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

This is probably related to my comment above.

rm -rf $(TARNAME)
Copy link

Choose a reason for hiding this comment

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

I would not generate the localization data files in the same directory as where the source tarball's content is stored, just to avoid some interdependency between these two unrelated targets.

mkdir $(TARNAME)
out/Release/icupkg -tb $(ICU_DATA_IN) $(TARNAME)/$(ICU_DATA_BE)
Copy link

Choose a reason for hiding this comment

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

$(ICU_DATA_IN) needs to have been generated, which is not necessarily the case depending on how node has been built (with or without icu support). What do you think about having these targets depending on icpukg and icudata instead of on node?

out/Release/icupkg -tl $(ICU_DATA_IN) $(TARNAME)/$(ICU_DATA_LE)
tar -cf $(BEDATATAR) $(TARNAME)/$(ICU_DATA_BE)
gzip -f -9 $(BEDATATAR)
tar -cf $(LEDATATAR) $(TARNAME)/$(ICU_DATA_LE)
gzip -f -9 $(LEDATATAR)
rm -rf $(TARNAME) $(LEDATATAR) $(BEDATATAR)
10 changes: 5 additions & 5 deletions tools/icu/icu-generic.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
'direct_dependent_settings': {
'defines': [
'UCONFIG_NO_CONVERSION=1',
'UCONFIG_NO_LEGACY_CONVERSION=1',
'UCONFIG_NO_IDNA=1',
'UCONFIG_NO_TRANSLITERATION=1',
'UCONFIG_NO_REGULAR_EXPRESSIONS=1',
]
},
},
Expand All @@ -30,12 +34,8 @@
'toolsets': [ 'host', 'target' ],
'direct_dependent_settings': {
'defines': [
'UCONFIG_NO_LEGACY_CONVERSION=1',
'UCONFIG_NO_IDNA=1',
'UCONFIG_NO_TRANSLITERATION=1',
'UCONFIG_NO_SERVICE=1',
'UCONFIG_NO_REGULAR_EXPRESSIONS=1',
'U_ENABLE_DYLOAD=0',
'UCONFIG_NO_SERVICE=1',
Copy link

Choose a reason for hiding this comment

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

I don't remember exactly what is the goal of these changes. Is it to save space in the target executable and in memory?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the change was here because it's not possible to do the endian swap without these settings. But again, i thought NPM installed data was the direction.

'U_STATIC_IMPLEMENTATION=1',
# Don't need std::string in API.
# Also, problematic: <http://bugs.icu-project.org/trac/ticket/11333>
Expand Down