Skip to content

Conversation

@haydenroche5
Copy link
Contributor

@haydenroche5 haydenroche5 commented Feb 8, 2022

This commit moves all the Windows Visual Studio files into the windows/
subdirectory. windows/ contains separate subdirectories and solutions for FIPS
140-2 and non-FIPS cases. I've also updated the documentation.

Depends on wolfSSL/wolfssl#4840.

@haydenroche5 haydenroche5 self-assigned this Feb 8, 2022
@haydenroche5 haydenroche5 force-pushed the visual_studio_improvements branch 2 times, most recently from 74a2eee to 55389ee Compare February 8, 2022 18:54
@haydenroche5 haydenroche5 force-pushed the visual_studio_improvements branch from 55389ee to f95f058 Compare February 9, 2022 20:40
This commit moves all the Windows Visual Studio files into the windows/
subdirectory. windows/ contains separate subdirectories and solutions for FIPS
140-2, FIPS Ready, and non-FIPS cases. I've also updated the documentation.
@haydenroche5 haydenroche5 force-pushed the visual_studio_improvements branch from f95f058 to 67c3740 Compare February 10, 2022 21:07
@julek-wolfssl
Copy link
Member

Initial pass through looks good. Is there a reason to make separate solutions instead of having a single solution and add configurations on top?

@haydenroche5
Copy link
Contributor Author

Initial pass through looks good. Is there a reason to make separate solutions instead of having a single solution and add configurations on top?

No particular reason. That actually sounds better. I'll look into it.

@julek-wolfssl
Copy link
Member

It would be great to have the base configurations and then have the FIPS solutions build on top of that. That way changes will need to be made only to the base configurations.

@haydenroche5
Copy link
Contributor Author

It would be great to have the base configurations and then have the FIPS solutions build on top of that. That way changes will need to be made only to the base configurations.

Yeah, I agree. Will look into it.

Instead of different solutions for FIPS 140-2, FIPS Ready, and
non-FIPS, use one solution with multiple configurations.
@haydenroche5 haydenroche5 force-pushed the visual_studio_improvements branch from 1ad8708 to b953330 Compare February 23, 2022 18:14
Copy link
Member

@julek-wolfssl julek-wolfssl left a comment

Choose a reason for hiding this comment

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

Great work on improving Windows VS solution support!

  1. As we discussed already. Please try to re-use configurations where possible. You found that you could use property sheets to that end which I think is great. Maintaining 12 distinct configurations (actually 24 due to x86 and x64 platforms) is not feasible in the long run.
    https://docs.microsoft.com/en-us/cpp/build/create-reusable-property-configurations?view=msvc-170
  2. I think you didn't clean up the test project files in the test directory.
  3. In the *.vcxproj files you specified v142 as the PlatformToolset. This corresponds to Visual Studio 2019. While I also would like to go back to 2019, it appears that VS 2022 has been released and I think its good practice to set the defaults to the latest version. This will avoid reports of customers trying to compile the solution with a PlatformToolset that they don't have installed.
    PlatformToolset list: https://docs.microsoft.com/en-us/cpp/build/how-to-modify-the-target-framework-and-platform-toolset?view=msvc-170

Any properties that would have been duplicated across configs
are now held in property sheets.
@haydenroche5
Copy link
Contributor Author

@julek-wolfssl I believe I've addressed all your feedback, but let me know if I missed anything / if you have any new comments.

Copy link
Member

@julek-wolfssl julek-wolfssl left a comment

Choose a reason for hiding this comment

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

@haydenroche5 These changes are absolutely great! Amazing work! The only thing missing is you somehow deleted windows/README.md. Please restore it and add a # Development section with a note about the property sheets (i.e. that all config needs to go the lowest possible property sheet and the properties of specific configs should not be modified past what is already in them).

@haydenroche5
Copy link
Contributor Author

@haydenroche5 These changes are absolutely great! Amazing work!

Thanks!

The only thing missing is you somehow deleted windows/README.md. Please restore it and add a # Development section with a note about the property sheets (i.e. that all config needs to go the lowest possible property sheet and the properties of specific configs should not be modified past what is already in them).

Thanks for the catch. Added it back and added a development section.

Copy link
Member

@julek-wolfssl julek-wolfssl left a comment

Choose a reason for hiding this comment

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

Great! I'm really liking the use of property sheets. We need to get this in wolfSSL ASAP. Over to @cconlon for a final review.

@cconlon cconlon merged commit 40eb265 into wolfSSL:master Mar 2, 2022
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