Skip to content

Update build system for more generic include paths #3781

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.

Sign up for GitHub

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

Closed
jackhorton wants to merge 3 commits into from
Closed

Update build system for more generic include paths #3781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9660b25
Modify build system to only set the ICU include folder in one place
jackhorton Sep 21, 2017
1cbb2f9
Update bytecode GUID
jackhorton Sep 21, 2017
f79657b
Addressing review comments
jackhorton Sep 21, 2017
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
7 changes: 6 additions & 1 deletion Build/Chakra.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@
<RuntimeLibrary Condition="'$(Configuration)'=='Debug' AND '$(RuntimeLib)'=='static_library'">MultiThreadedDebug</RuntimeLibrary>
<AdditionalIncludeDirectories>
$(ChakraCoreRootDirectory)\lib\common\placeholder;
$(IntDir)..\CoreManifests\
$(IntDir)..\CoreManifests;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<AdditionalIncludeDirectories Condition="'$(IntlICU)'=='true'">
$(IcuIncludeDir)\common;
$(IcuIncludeDir)\i18n;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
</ClCompile>
Expand Down
3 changes: 2 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,8 @@ include_directories(
pal
pal/inc
pal/inc/rt
${ICU_INCLUDE_PATH}
${ICU_INCLUDE_PATH}/common
Copy link
Collaborator

@obastemur obastemur Sep 22, 2017

Choose a reason for hiding this comment

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

why this is needed?

Copy link
Contributor Author

@jackhorton jackhorton Sep 22, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@obastemur obastemur Sep 22, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dilijev dilijev Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@obastemur obastemur Sep 22, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@obastemur obastemur Sep 22, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jackhorton jackhorton Sep 22, 2017

Choose a reason for hiding this comment

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

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

  1. put the paths in the #includes
include_directories(
  ...
  ${ICU_INCLUDE_PATH}
  )
#include "common/unicode/uloc.h"
#include "i18n/header.h"
  1. 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.

Copy link
Contributor Author

@jackhorton jackhorton Sep 22, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@dilijev dilijev Sep 23, 2017

Choose a reason for hiding this comment

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

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

Prefer the former because it takes more changes?

Copy link
Contributor Author

@jackhorton jackhorton Sep 24, 2017

Choose a reason for hiding this comment

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

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

${ICU_INCLUDE_PATH}/i18n
)

if(ICU_INCLUDE_PATH)
Expand Down
1 change: 0 additions & 1 deletion bin/ChakraCore/ChakraCore.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
$(ChakraCoreRootDirectory)Lib\Jsrt;
$(IntDir);
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
$(MSBuildThisFileDirectory)..\..\lib\JITClient;
%(AdditionalIncludeDirectories);
</AdditionalIncludeDirectories>
Expand Down
3 changes: 1 addition & 2 deletions lib/Backend/Chakra.Backend.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
$(MSBuildThisFileDirectory)..\Parser;
$(MSBuildThisFileDirectory)..\WasmReader;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand Down Expand Up @@ -483,4 +482,4 @@
<Import Project="$(VCTargetsPath)\BuildCustomizations\masm.targets" />
<Import Project="$(BuildConfig_ARMASM_Path)armasm.targets" />
</ImportGroup>
</Project>
</Project>
3 changes: 1 addition & 2 deletions lib/JITServer/Chakra.JITServer.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
$(MSBuildThisFileDirectory)..\Backend\$(PlatformPathNameAlt);
$(MSBuildThisFileDirectory)..\Backend;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand All @@ -57,4 +56,4 @@
</ItemDefinitionGroup>
<Import Project="$(BuildConfigPropsPath)Chakra.Build.targets" Condition="exists('$(BuildConfigPropsPath)Chakra.Build.targets')" />
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
</Project>
</Project>
3 changes: 1 addition & 2 deletions lib/Jsrt/Chakra.Jsrt.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
$(MSBuildThisFileDirectory);
$(MSBuildThisFileDirectory)..\Runtime;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
$(MSBuildThisFileDirectory)..\JITClient;
$(MSBuildThisFileDirectory)..\Runtime\ByteCode;
$(MSBuildThisFileDirectory)..\Common;
Expand Down Expand Up @@ -91,4 +90,4 @@
</ItemGroup>
<Import Project="$(BuildConfigPropsPath)Chakra.Build.targets" Condition="exists('$(BuildConfigPropsPath)Chakra.Build.targets')" />
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
</Project>
</Project>
3 changes: 1 addition & 2 deletions lib/Jsrt/Core/Chakra.Jsrt.Core.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
$(MSBuildThisFileDirectory)..;
$(MSBuildThisFileDirectory)..\..\Runtime;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(MSBuildThisFileDirectory)..\..\Runtime\ByteCode;
$(MSBuildThisFileDirectory)..\..\Common;
Expand All @@ -50,4 +49,4 @@
</ItemGroup>
<Import Project="$(BuildConfigPropsPath)Chakra.Build.targets" Condition="exists('$(BuildConfigPropsPath)Chakra.Build.targets')" />
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
</Project>
</Project>
1 change: 0 additions & 1 deletion lib/Parser/Chakra.Parser.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
$(MSBuildThisFileDirectory)..\Runtime;
$(MSBuildThisFileDirectory)..\Runtime\ByteCode;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
$(ChakraManifestsIncludeDirectory);
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
Expand Down
1 change: 0 additions & 1 deletion lib/Runtime/Base/Chakra.Runtime.Base.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
$(MSBuildThisFileDirectory)..\..\Runtime\ByteCode;
$(MSBuildThisFileDirectory)..\..\WasmReader;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
$(MSBuildThisFileDirectory)..\..\JITClient;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
Expand Down
4 changes: 2 additions & 2 deletions lib/Runtime/ByteCode/ByteCodeCacheReleaseFileVersion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
//-------------------------------------------------------------------------------------------------------
// NOTE: If there is a merge conflict the correct fix is to make a new GUID.

// {5C44723E-F7DF-4397-A24B-AC1A90D221F9}
// {131ae129-1863-4b97-b9b2-4802d60acefb}
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 } };
Copy link
Contributor

@dilijev dilijev Sep 21, 2017

Choose a reason for hiding this comment

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

👍 Thanks!

Note: Fixes #3773

3 changes: 1 addition & 2 deletions lib/Runtime/ByteCode/Chakra.Runtime.ByteCode.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
$(MSBuildThisFileDirectory)..\..\Backend;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand Down Expand Up @@ -105,4 +104,4 @@
</ItemGroup>
<Import Project="$(BuildConfigPropsPath)Chakra.Build.targets" Condition="exists('$(BuildConfigPropsPath)Chakra.Build.targets')" />
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
</Project>
</Project>
3 changes: 1 addition & 2 deletions lib/Runtime/Debug/Chakra.Runtime.Debug.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
$(MSBuildThisFileDirectory)..\..\Runtime\ByteCode;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand Down Expand Up @@ -113,4 +112,4 @@
<Import Project="$(VCTargetsPath)\BuildCustomizations\masm.targets" />
<Import Project="$(BuildConfig_ARMASM_Path)armasm.targets" />
</ImportGroup>
</Project>
</Project>
3 changes: 1 addition & 2 deletions lib/Runtime/Language/Chakra.Runtime.Language.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
$(MSBuildThisFileDirectory)..\..\Runtime\Math;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand Down Expand Up @@ -311,4 +310,4 @@
<Import Project="$(VCTargetsPath)\BuildCustomizations\masm.targets" />
<Import Project="$(BuildConfig_ARMASM_Path)armasm.targets" />
</ImportGroup>
</Project>
</Project>
1 change: 0 additions & 1 deletion lib/Runtime/Library/Chakra.Runtime.Library.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
$(MSBuildThisFileDirectory)..\..\Runtime\ByteCode;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common; <!-- // TODO (doilij): At end of IntlICU development, remove this if it is not needed here. -->
$(MSBuildThisFileDirectory)..\Math;
$(ChakraManifestsIncludeDirectory);
$(ManifestsInboxIncludeDirectory);
Expand Down
3 changes: 1 addition & 2 deletions lib/Runtime/Math/Chakra.Runtime.Math.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
$(MSBuildThisFileDirectory)..\..\Backend;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand Down Expand Up @@ -69,4 +68,4 @@
</ItemGroup>
<Import Project="$(BuildConfigPropsPath)Chakra.Build.targets" Condition="exists('$(BuildConfigPropsPath)Chakra.Build.targets')" />
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
</Project>
</Project>
1 change: 0 additions & 1 deletion lib/Runtime/PlatformAgnostic/Chakra.Runtime.PlatformAgnostic.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
$(MSBuildThisFileDirectory)..\..\Runtime\ByteCode;
$(MSBuildThisFileDirectory)..\..\Backend;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(MSBuildThisFileDirectory)..;$(MSBuildThisFileDirectory);
%(AdditionalIncludeDirectories)
Expand Down
3 changes: 1 addition & 2 deletions lib/Runtime/Types/Chakra.Runtime.Types.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
$(MSBuildThisFileDirectory)..\..\Runtime\ByteCode;
$(MSBuildThisFileDirectory)..\..\JITClient;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand Down Expand Up @@ -115,4 +114,4 @@
</ItemGroup>
<Import Project="$(BuildConfigPropsPath)Chakra.Build.targets" Condition="exists('$(BuildConfigPropsPath)Chakra.Build.targets')" />
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
</Project>
</Project>
3 changes: 1 addition & 2 deletions lib/WasmReader/Chakra.WasmReader.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
$(MSBuildThisFileDirectory)..\Backend;
$(MSBuildThisFileDirectory)..\JITClient;
$(ChakraJITIDLIntDir);
$(IcuIncludeDir)\common;
%(AdditionalIncludeDirectories)
</AdditionalIncludeDirectories>
<PrecompiledHeader>Use</PrecompiledHeader>
Expand Down Expand Up @@ -76,4 +75,4 @@
</ItemGroup>
<Import Project="$(BuildConfigPropsPath)Chakra.Build.targets" Condition="exists('$(BuildConfigPropsPath)Chakra.Build.targets')" />
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
</Project>
</Project>