Update build system for more generic include paths #3781

jackhorton · 2017-09-21T16:41:07Z

This will eventually be useful for standardizing how embedded ICU works across windows/xplat and ChakraCore/Node-ChakraCore. This also updates the bytecode version as a precaution.

Fixes #3773

msftclas · 2017-09-21T16:41:10Z

@jackhorton,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

jackhorton · 2017-09-21T16:46:59Z

Fixes #3773

Also, has there ever been a discussion about refactoring all of these include directories? The CMakeLists and vcxprojs look totally different here, and at the cost of possibly introducing some header naming conflicts (which seems unlikely to me right now), I think it would be nice if we could simplify the system at least enough for every project to look the same. If its an item at all, it would be fairly low-priority, but still useful IMO

jackhorton · 2017-09-21T16:55:07Z

Oh and lastly, @dilijev , I think I removed the default lib and include path if ICU was true mistakenly, I can add that back in if you still need it. I can also make some of the conditions more strict (like IntlICU==true AND IcuIncludePath=='' for setting default include paths, just to be sure)

MSLaguana · 2017-09-21T18:38:55Z

Build/Chakra.Build.Paths.props

@@ -11,9 +11,9 @@
  </PropertyGroup>
  <PropertyGroup Label="ICU">
    <!-- IcuLibDir can be set in environment or with "/p:IcuLibDir=..." -->
-    <IcuLibDir Condition="'$(IcuLibDir)'=='' AND '$(Configuration)'=='Debug'">$(ChakraCoreRootDirectory)\deps\static\icu-small\debug</IcuLibDir>
-    <IcuLibDir Condition="'$(IcuLibDir)'=='' AND ('$(Configuration)'=='Test' or '$(Configuration)'=='Release')">$(ChakraCoreRootDirectory)\deps\static\icu-small\release</IcuLibDir>
+    <!-- <IcuLibDir Condition="'$(IcuLibDir)'=='' AND '$(Configuration)'=='Debug'">$(ChakraCoreRootDirectory)\deps\static\icu-small\debug</IcuLibDir> -->


What is the purpose of keeping these lines around commented out? Are they intended as documentation of how to specify the IcuLibDir parameter to msbuild?

The real reason is because I commented them out to ensure nothing was being set behind my back and then made the pull request without remembering to either comment their removal better or add them back. My comment above asking doug what to do here is why it hasn't been fixed yet.

Yeah, let's keep them for now.

dilijev · 2017-09-21T21:05:07Z

lib/Runtime/ByteCode/ByteCodeCacheReleaseFileVersion.h

 const GUID byteCodeCacheReleaseFileVersion =
-{ 0x5C44723E, 0xF7DF, 0x4397, { 0xA2, 0x4B, 0xAC, 0x1A, 0x90, 0xD2, 0x21, 0xF9 } };
+{ 0x131ae129, 0x1863, 0x4b97, { 0xb9, 0xb2, 0x48, 0x02, 0xd6, 0x0a, 0xce, 0xfb } };


👍 Thanks!

Note: Fixes #3773

dilijev · 2017-09-21T21:07:46Z

@jackhorton

I think I removed the default lib and include path if ICU was true mistakenly, I can add that back in if you still need it.

It is convenient to have them. Let's clean up things like that only after this work lands to make development easier for the time being.

dilijev

See other comments.

dilijev · 2017-09-21T21:09:51Z

@digitalinfinity Any objections to moving these dependencies into a single place at the top level instead of having it in 1/2 of our vcxproj files as I had done before? IIRC we had a conversation about minimal dependencies at some point and that was the reason for that somewhat unweildy decision. FWIW I like this better and it matches the way we do it for CMake/xplat which is one reason to unify.

jackhorton · 2017-09-21T22:17:41Z

@dilijev Re-added the default lib and include values

digitalinfinity · 2017-09-21T22:33:50Z

@dilijev no, no objections as long as whatever technique you use works fine with the Windows build systems

obastemur · 2017-09-22T00:53:47Z

CMakeLists.txt

@@ -456,7 +456,8 @@ include_directories(
    pal
    pal/inc
    pal/inc/rt
-    ${ICU_INCLUDE_PATH}
+    ${ICU_INCLUDE_PATH}/common


why this is needed?

Because @dilijev is working on things right now that have headers in /source/i18n in addition to source/common. Its easier if we normalize everything off of just icu/source

Well, this is not a trivial change. Better if we keep it as is. There is a reason they kept the header files in two sep. folders. We shouldn't normalize them.

Not sure what you mean. ICU_INCLUDE_PATH will have a known directory tree with files in known places. The dependency on ICU headers is effectively the directory that needs to be specified. In my WIP*, I'm adding ${ICU_INCLUDE_PATH}/i18n because that's where those headers are.

* see: https://github.com/dilijev/ChakraCore/tree/intl-numfmt and this commit in particular: dilijev@a04cde6

https://github.com/dilijev/ChakraCore/blob/a04cde6ce70c1a9d92f2add6dc55b51a19a98cb4/lib/Runtime/Base/Chakra.Runtime.Base.vcxproj#L31-L32

If not this strategy, what would you propose? Have the build system specify two directories that are within the same parent folder for semantically the same dependency?

1 - This is the place where you set the search path for compiler. If you put both i18n and common separately, you would normalize all the header files under.

How it's different than copying all the header files from those folders into a common folder?
Well, worst case, it might break and we won't even notice.

What's wrong with #include <i18n/THIS_FILE> and #include <common/THAT_FILE> ?

2 - ICU is optional. There is a reason we created a variable for ICU_INCLUDE_PATH... Once ${ICU_INCLUDE_PATH} is not defined, what exactly do expect from compiler for ${ICU_INCLUDE_PATH}/common or ${ICU_INCLUDE_PATH}/.... ??

If we didn't do this for Windows project files, well, it's time we make it right?

Alternatively, only if INTL is enabled, add ${ICU_INCLUDE_PATH}../i18/.. Mind ../ because ICU_INCLUDE_PATH == common already.

However, we should track from CMAKE and make sure INTL is enabled before adding that.

So, we won't be updating build scripts command lines (it's not just us!) and make the header sharing limited to INTL usage. I guess, this is what you want?

I think that both of the following are fine, all things being considered equal:

put the paths in the #includes

include_directories( ... ${ICU_INCLUDE_PATH} )

#include "common/unicode/uloc.h" #include "i18n/header.h"

Scope the include

if(SET ${ICU_INCLUDE_PATH}) include_directories( ${ICU_INCLUDE_PATH}/common ${ICU_INCLUDE_PATH/i18n ) endif()

The latter takes fewer changes, so I would probably prefer the former at least for now.

Note that ICU_INCLUDE_PATH will only be set when we are not using the system ICU -- it will be worth looking at where the i18n headers are when theyre installed on the system and probably choose whatever is necessary to conform to that

The latter takes fewer changes, so I would probably prefer the former at least for now.

Prefer the former because it takes more changes?

I have no idea what I was on about there, I prefer solution #2.

dilijev

LGTM
Do note that there might be a linux build dependency that you'll need to consider as @obastemur mentioned above. Not sure it's worth blocking this PR on that point unless it fails to build in some scenario not covered by the CI.

dilijev · 2017-09-25T22:56:59Z

Note for @jackhorton and @obastemur:

I'm closing this PR and will include @jackhorton's changes to the Windows build (and GUID update) in my next PR. This is because the Windows changes do not regress any current scenario, and are necessary to unblock my work. The Linux changes will be left to a future change after the proper path forward can be investigated in more detail.

@jackhorton

Merge pull request #3809 from dilijev:intl-numfmt Closes #3661 (platform.currencyDigits) Closes #3664 (platform.formatNumber) Closes #3716 (platform.cacheNumberFormat) Closes #3670 (Intl.NumberFormat ctor) Closes #3671 (Intl.NumberFormat.prototype.format) Includes some of @jackhorton's changes from PR #3781: * Windows ICU dependency updates * GUID update (Fixes #3773)

jackhorton added 2 commits September 20, 2017 20:48

Modify build system to only set the ICU include folder in one place

9660b25

Update bytecode GUID

1cbb2f9

msftclas added the cla-not-required label Sep 21, 2017

jackhorton requested a review from dilijev September 21, 2017 16:47

jackhorton added Dev Experience Intl-ICU labels Sep 21, 2017

jackhorton mentioned this pull request Sep 21, 2017

Added --with-intl flag on linux, android, and macOS nodejs/node-chakracore#394

Closed

2 tasks

MSLaguana reviewed Sep 21, 2017

View reviewed changes

dilijev reviewed Sep 21, 2017

View reviewed changes

dilijev suggested changes Sep 21, 2017

View reviewed changes

dilijev requested a review from digitalinfinity September 21, 2017 21:08

Addressing review comments

f79657b

jackhorton force-pushed the windows-icu branch from 806e2ea to f79657b Compare September 21, 2017 22:16

obastemur reviewed Sep 22, 2017

View reviewed changes

dilijev approved these changes Sep 22, 2017

View reviewed changes

dilijev closed this Sep 25, 2017

dilijev mentioned this pull request Sep 27, 2017

Intl-ICU: implement Intl.NumberFormat under ICU #3809

Merged

jackhorton deleted the windows-icu branch March 7, 2018 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update build system for more generic include paths #3781

Update build system for more generic include paths #3781

jackhorton commented Sep 21, 2017 •

edited by dilijev

Loading

msftclas commented Sep 21, 2017

jackhorton commented Sep 21, 2017

jackhorton commented Sep 21, 2017

MSLaguana Sep 21, 2017

jackhorton Sep 21, 2017

dilijev Sep 21, 2017

dilijev Sep 21, 2017

dilijev commented Sep 21, 2017

dilijev left a comment

dilijev commented Sep 21, 2017 •

edited

Loading

jackhorton commented Sep 21, 2017

digitalinfinity commented Sep 21, 2017

obastemur Sep 22, 2017

jackhorton Sep 22, 2017

obastemur Sep 22, 2017

dilijev Sep 22, 2017 •

edited

Loading

obastemur Sep 22, 2017

obastemur Sep 22, 2017

jackhorton Sep 22, 2017

jackhorton Sep 22, 2017

dilijev Sep 23, 2017

jackhorton Sep 24, 2017

dilijev left a comment •

edited

Loading

dilijev commented Sep 25, 2017

Update build system for more generic include paths #3781

Update build system for more generic include paths #3781

Conversation

jackhorton commented Sep 21, 2017 • edited by dilijev Loading

msftclas commented Sep 21, 2017

jackhorton commented Sep 21, 2017

jackhorton commented Sep 21, 2017

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

dilijev commented Sep 21, 2017

dilijev left a comment

Choose a reason for hiding this comment

dilijev commented Sep 21, 2017 • edited Loading

jackhorton commented Sep 21, 2017

digitalinfinity commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev Sep 22, 2017 • 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

dilijev left a comment • edited Loading

Choose a reason for hiding this comment

dilijev commented Sep 25, 2017

jackhorton commented Sep 21, 2017 •

edited by dilijev

Loading

dilijev commented Sep 21, 2017 •

edited

Loading

dilijev Sep 22, 2017 •

edited

Loading

dilijev left a comment •

edited

Loading