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

Simplify windows build #2632

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Conversation

philippun1
Copy link
Contributor

@philippun1 philippun1 commented Jun 14, 2023

This MR simplifies the windows build with these updates:

  • add a openssl.props to replace the openssl path magic strings
  • update the platform toolset to ClangCL, which aligns it with the tpm2-provider project. (Also v141_clang_c2 is experimental)
  • generalize the windows target platform to 10.0 (which will make it pick the latest installed one)
  • update the VS version in the .sln file to 17 (the same as in the tpm2-provider project)
  • update image to VS 2019 in .appveyor.yml
  • update INSTALL.md to reflect ClangCL / VS 2019 changes

No changes to actual source code are made.

@Superhepper
Copy link
Contributor

Superhepper commented Sep 2, 2023

This works nicely! I tested it with Visual Studio 2022 on Windows 11. If you do not mind I would like to add a suggestion to this.

I would like to see a post build step that creates the "VERSION" file. I did an experiment and added a 'Post-Build Event' to the mu project which just executed the following call git describe --tags --always --dirty> "$(SolutionDir)VERSION". By doing this the solution will behave similar to the VERSION file is created in bootstrap. And by doing this as Post-Build it should still be possible to build everything even though git is not installed.

And in a future PR it would really nice to see if the version information could be built into LIBs and DLLs them self's using resource files.

@Superhepper
Copy link
Contributor

Superhepper commented Nov 3, 2023

I think you will need to add the following lines:

environment:
  matrix:
    - toolchain: clang

to the .appveyor.yml file in order for the tests to work. But I can be wrong.

And I do not think you should add a .gitignore file because from what I have seen in the other build scripts it is generated by the build system. So just as with the VERSION file that is something the solution or some of the projects files will have to handle.

@philippun1 philippun1 force-pushed the windows-build branch 3 times, most recently from 1e00ddf to 2ee51da Compare November 4, 2023 23:16
@philippun1
Copy link
Contributor Author

@Superhepper Updating the image to VS 2019 did the trick.
I also removed the .gitignore. It is only being generated with the makefile though, so it will not be generated if compiling under Windows.

@Superhepper
Copy link
Contributor

I think you might also need to update the documentation in INSTALL.md

@AndreasFuchsTPM
Copy link
Member

In general this looks good to me and I highly appreciate the work.
One note though:
It seems like not all commits will pass CI, since the Appveyor is updated in a later commit than the former. I might be wrong here.

Just in general, I want to make sure that each and every commit would be able to pass CI.

@philippun1
Copy link
Contributor Author

@AndreasFuchsTPM I rebased onto latest master, did all the tests run now?

@Superhepper
Copy link
Contributor

Superhepper commented Nov 30, 2023

No what I think @AndreasFuchsTPM ment was that the first commit changes WindowsTargetPlatformVersion and PlatformToolset and the solution file to have VS2019 as the target compiler version. But the change to CI(appveyor) is made two commits later so it will not be possible to build the two previous commits in CI. It might be better to move the 'appveyor' commit into the first commit that updates the targets so all the commits can be built in CI.

00a7589 - Cannot be built in CI. Appveyor points to VS2017
d27cada - Cannot be built in CI. Appveyor points to VS2017
36d5aec - Appveyor is updated, can be built in CI.
abc907e - Can be built in CI
277831e - Can be built in CI
7d8ce7f - Can be built in CI

philippun1 and others added 4 commits December 1, 2023 19:28
Signed-off-by: Philipp Unger <philipp.unger@tsep.com>
Signed-off-by: Philipp Unger <philipp.unger@tsep.com>
Signed-off-by: Philipp Unger <philipp.unger.1988@gmail.com>
Signed-off-by: Philipp Unger <philipp.unger@tsep.com>
@philippun1
Copy link
Contributor Author

@Superhepper Thanks for the clarification. I squashed the appveyor changes into the first commit.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fcff6da) 82.55% compared to head (4fae5cc) 82.59%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2632      +/-   ##
==========================================
+ Coverage   82.55%   82.59%   +0.04%     
==========================================
  Files         369      369              
  Lines       43111    43124      +13     
==========================================
+ Hits        35590    35619      +29     
+ Misses       7521     7505      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AndreasFuchsTPM AndreasFuchsTPM merged commit 0b0a58e into tpm2-software:master Jan 10, 2024
28 checks passed
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.

3 participants