Skip to content
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

Tpm2-tss build with VS2019 + Windows ARM64 #2182

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

achal1012
Copy link

No description provided.

@AndreasFuchsTPM
Copy link
Member

First of all: Our .gitignore file is autogenerated. We don't want to add it just for Windows env. IMHO windows should be able to know which files it generates by itself quite nicely without need for a .gitignore, or am I wrong ?

Then: Are you still working on this (since this was a "draft") or have you given up ?

@achal1012
Copy link
Author

First of all: Our .gitignore file is autogenerated. We don't want to add it just for Windows env. IMHO windows should be able to know which files it generates by itself quite nicely without need for a .gitignore, or am I wrong ?

Then: Are you still working on this (since this was a "draft") or have you given up ?

Hi @AndreasFuchsTPM you are right; gitignore was required for our dev setup.
I have no intention to merge this in unless the authours of this repo want me to; I just created this draft PR so my teammates can look at the diff and build this for ARM64 platform. :)

@williamcroberts
Copy link
Member

I guess we could take the changes to the project files. For the gitignore you can add it to .git/info/exclude. Everyone who works on it can just drop it in there. This is per: https://www.atlassian.com/git/tutorials/saving-changes/gitignore#personal-git-ignore-rules

@achal1012
Copy link
Author

I guess we could take the changes to the project files. For the gitignore you can add it to .git/info/exclude. Everyone who works on it can just drop it in there. This is per: https://www.atlassian.com/git/tutorials/saving-changes/gitignore#personal-git-ignore-rules

Sure let me fix the changes and publish the pull request.

@achal1012 achal1012 marked this pull request as ready for review June 16, 2022 21:09
@williamcroberts
Copy link
Member

@achal1012 I took the liberty of fixing the merge conflicts since we took so long to review this again. Could you sign off your commits though to make the DCO bot happy?

@AndreasFuchsTPM
Copy link
Member

IMHO we should squash the two commits into a single one.

@achal1012 Could you provide some reference to the target platform you are using so we could setup our own testing env for this ?

@achal1012
Copy link
Author

Hi @williamcroberts: I am unable to approve this PR as github is not letting me. And I apologize for no updates on this PR, I changed employers and lost my setup and test bench.

@AndreasFuchsTPM I tested this on Ampere platform with internal windows ARM564 build, and unfortunately I don't have access to that now.

@williamcroberts
Copy link
Member

Hi @williamcroberts: I am unable to approve this PR as github is not letting me.

You don't need to approve anything, You can still sign off your commits. In the Tests section for your PR you'll see a DCO thing that shows as failed, follow it's steps:

And I apologize for no updates on this PR, I changed employers and lost my setup and test bench.

We'll as long as it didn't break the normal x86-64 builds so I am fine with pulling it as is as experimental ARM64 support.

acvelani and others added 2 commits November 15, 2022 08:26
Signed-off-by: Achal Velani <achal.velani@gmail.com>
Signed-off-by: Achal Velani <achal.velani@gmail.com>
@achal1012
Copy link
Author

You don't need to approve anything, You can still sign off your commits. In the Tests section for your PR you'll see a DCO thing that shows as failed, follow it's steps:

I followed the steps and pushed the change. Can you check if its okay now?

@williamcroberts
Copy link
Member

You don't need to approve anything, You can still sign off your commits. In the Tests section for your PR you'll see a DCO thing that shows as failed, follow it's steps:

I followed the steps and pushed the change. Can you check if its okay now?

DCO bot happy, lets let CI spin and I can pull this in for 4.0 release.

@williamcroberts williamcroberts added this to the Version 4.0 milestone Nov 16, 2022
Copy link
Member

@AndreasFuchsTPM AndreasFuchsTPM left a comment

Choose a reason for hiding this comment

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

AppVeyor complains that
esys_crypto_ossl.c(11,10): fatal error : 'openssl/rand.h' file not found [C:\projects\tpm2-tss\src\tss2-esys\tss2-esys.vcxproj]

I see that include-paths were changed for OpenSSL.
I don't know though, how to nicely handle this.

</ClCompile>
<Link>
<TargetMachine>MachineX86</TargetMachine>
<GenerateDebugInformation>true</GenerateDebugInformation>
<SubSystem>Windows</SubSystem>
<AdditionalDependencies>$(OutDir)\tss2-mu.lib;$(OutDir)\tss2-sys.lib;$(OutDir)\tss2-tctildr.lib;C:\OpenSSL-v11-Win32\lib\libcrypto.lib;C:\OpenSSL-v11-Win32\lib\libcrypto.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalDependencies>$(OutDir)\tss2-mu.lib;$(OutDir)\tss2-sys.lib;$(OutDir)\tss2-tctildr.lib;C:\OpenSSL.1.1.1506.73\$(Platform)\$(Configuration)\lib\libcrypto.lib;%(AdditionalDependencies)</AdditionalDependencies>
Copy link
Member

Choose a reason for hiding this comment

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

This change apparently breaks our Appveyor-CI

</ClCompile>
<Link>
<TargetMachine>MachineX86</TargetMachine>
<GenerateDebugInformation>true</GenerateDebugInformation>
<SubSystem>Windows</SubSystem>
<EnableCOMDATFolding>true</EnableCOMDATFolding>
<OptimizeReferences>true</OptimizeReferences>
<AdditionalDependencies>$(OutDir)\tss2-mu.lib;$(OutDir)\tss2-sys.lib;$(OutDir)\tss2-tctildr.lib;C:\OpenSSL-v11-Win32\lib\libcrypto.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalDependencies>$(OutDir)\tss2-mu.lib;$(OutDir)\tss2-sys.lib;$(OutDir)\tss2-tctildr.lib;C:\OpenSSL.1.1.1506.73\$(Platform)\$(Configuration)\lib\libcrypto.lib;%(AdditionalDependencies)</AdditionalDependencies>
Copy link
Member

Choose a reason for hiding this comment

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

Same here

<ModuleDefinitionFile>$(SolutionDir)\lib\tss2-esys.def</ModuleDefinitionFile>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
<ClCompile>
<AdditionalIncludeDirectories>$(SolutionDir);$(SolutionDir)\src;$(SolutionDir)\include\tss2;$(SolutionDir)\src\tss2-mu;$(SolutionDir)\src\tss2-sys;$(SolutionDir)\src\tss2-esys;C:\OpenSSL-v11-Win64\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<AdditionalIncludeDirectories>$(SolutionDir);$(SolutionDir)\src;$(SolutionDir)\include\tss2;$(SolutionDir)\src\tss2-mu;$(SolutionDir)\src\tss2-sys;$(SolutionDir)\src\tss2-esys;C:\OpenSSL.1.1.1506.73\$(Platform)\$(Configuration)\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
Copy link
Member

Choose a reason for hiding this comment

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

And here

<PreprocessorDefinitions>NDEBUG;_WINDOWS;_USRDLL;TSS2ESYS_EXPORTS;MAXLOGLEVEL=6;strtok_r=strtok_s;OSSL;%(PreprocessorDefinitions)</PreprocessorDefinitions>
</ClCompile>
<Link>
<AdditionalDependencies>$(OutDir)\tss2-mu.lib;$(OutDir)\tss2-sys.lib;$(OutDir)\tss2-tctildr.lib;C:\OpenSSL-v11-Win64\lib\libcrypto.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalDependencies>$(OutDir)\tss2-mu.lib;$(OutDir)\tss2-sys.lib;$(OutDir)\tss2-tctildr.lib;C:\OpenSSL.1.1.1506.73\$(Platform)\$(Configuration)\lib\libcrypto.lib;%(AdditionalDependencies)</AdditionalDependencies>
Copy link
Member

Choose a reason for hiding this comment

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

and here

@williamcroberts
Copy link
Member

AppVeyor complains that esys_crypto_ossl.c(11,10): fatal error : 'openssl/rand.h' file not found [C:\projects\tpm2-tss\src\tss2-esys\tss2-esys.vcxproj]

I see that include-paths were changed for OpenSSL. I don't know though, how to nicely handle this.

If it breaks appveyor we don't pull, @achal1012 if you can add your arm64 support and AppVeyor is happy thats good enough for me, but otherwise we won't be able to take this PR. I thought AppVeyor was happy at one point....

@williamcroberts williamcroberts removed this from the Version 4.0 milestone Nov 17, 2022
@achal1012
Copy link
Author

AppVeyor complains that esys_crypto_ossl.c(11,10): fatal error : 'openssl/rand.h' file not found [C:\projects\tpm2-tss\src\tss2-esys\tss2-esys.vcxproj]
I see that include-paths were changed for OpenSSL. I don't know though, how to nicely handle this.

If it breaks appveyor we don't pull, @achal1012 if you can add your arm64 support and AppVeyor is happy thats good enough for me, but otherwise we won't be able to take this PR. I thought AppVeyor was happy at one point....

Yes, I think here we have to use a build of OpenSSL that has the ARM64 libs as well.
C:\OpenSSL.1.1.1506.73
This was a hardcoded version I was using.

@achal1012
Copy link
Author

Can we keep this PR in draft till I can find some time to work on this? Or do we need to abandon it?

@williamcroberts
Copy link
Member

Can we keep this PR in draft till I can find some time to work on this? Or do we need to abandon it?

We can just leave it open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants