-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
Add ARM64 support #92
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about having this depend on ReactNative Windows for ARM64. I don't have much context about why building for ARM64 would require a dependency. Hopefully you can rework this to not have that dependency. I'd like to learn more.
@@ -90,6 +98,7 @@ | |||
<Import Project="..\..\packages\boost.1.68.0.0\build\boost.targets" Condition="Exists('..\..\packages\boost.1.68.0.0\build\boost.targets')" /> | |||
<Import Project="..\..\packages\boost_date_time-vc141.1.68.0.0\build\boost_date_time-vc141.targets" Condition="Exists('..\..\packages\boost_date_time-vc141.1.68.0.0\build\boost_date_time-vc141.targets')" /> | |||
<Import Project="..\..\packages\Microsoft.ChakraCore.vc140.1.11.20\build\native\Microsoft.ChakraCore.vc140.targets" Condition="Exists('..\..\packages\Microsoft.ChakraCore.vc140.1.11.20\build\native\Microsoft.ChakraCore.vc140.targets')" /> | |||
<Import Project="..\..\packages\ReactWindows.ChakraCore.ARM64.1.11.20\build\native\ReactWindows.ChakraCore.ARM64.targets" Condition="'$(Platform)' == 'ARM64' AND Exists('..\..\packages\ReactWindows.ChakraCore.ARM64.1.11.20\build\native\ReactWindows.ChakraCore.ARM64.targets')" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what's required to be imported for ARM64? These should not be coming from the ReactNative project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the ChakraCore team is not producing an ARM64-enabled NuGet. They don't plan to do so.
They did provide us with the actual binaries. We just had to package them ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have a discussion about this. If they are providing the binaries, then they will need to do so for each release. Why wouldn't ChakraCore then be creating the NuGet? Could we have the ChakraCore-Debugger pipeline create the separate NuGet for ARM64 rather than do this from ReactWindows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't ChakraCore then be creating the NuGet?
Can't say in detail. We'd have to ask them. They did say that, because ChakraCore is in "life-support" mode, they are reluctant to modify their release pipeline just to provide support for a new platform.
Could we have the ChakraCore-Debugger pipeline create the separate NuGet for ARM64 rather than do this from ReactWindows?
Is that:
- A separate ChakraCore|ARM64 NuGet?
It's already produced and released. Yes, updated binaries will need to be handed by them, but to have the CCDB pipeline produce updated ARM64 NuGets, they would already need to place them in a storage source accessible by the pipeline. What are we gaining out of this, and how does that block this PR? - A separate ChakraCore-Debugger|ARM64 NuGet?
What is the use for this? We currently bundle both x86 and x64 binaries in the same NuGet. We would then have to create separate NuGets for x86 and x64, shouldn't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate ChakraCore ARM64 NuGet. Regardless of who produced this NuGet, it doesn't have anything to do with ReactWindows other than it wants to be a consumer of the NuGet. Couldn't the name be updated to reflect that by using "Microsoft" instead of "ReactWindows"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Microsoft
is considered a reserved prefix, which should be only used for officially-released packages.
The "ReactWindows" prefix is used for many semi-internal (internally produced, but publicly available) packages.
Deciding on the naming convention of other projects' NuGet packages is out of the scope of this PR.
There is no need nor use for a new/renamed package, especially when the existing one is already also used by React Native Windows.
If the PR itself does not introduce any regression, it doesn't add any build or runtime dependencies on React Native, and does not affect any functional or conventional content of the code base, can it be merged as is?
Closing. React Native Windows has dumped ChakraCore-Debugger. |
Adds the necessary dependencies to release an ARM64 platform target.