Skip to content

updated the readme for issue #4#5

Open
mukeshbharath wants to merge 2 commits into
mcci-catena:mainfrom
mukeshbharath:issue4
Open

updated the readme for issue #4#5
mukeshbharath wants to merge 2 commits into
mcci-catena:mainfrom
mukeshbharath:issue4

Conversation

@mukeshbharath

Copy link
Copy Markdown
Contributor

Hello Terry, I have updated the readme for the tools/mccibooatloader_image regarding the clang compile issue faced.

Please check and merge.

@terrillmoore terrillmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution. The underlying issue is real -- clang on Windows needs the UCRT headers from the Windows SDK -- but this PR needs some rework before it can be merged.

  1. Terminology: "Visual Studio SDK" refers to the VS extensibility SDK, not the Windows SDK. The prerequisite here is the Windows SDK (specifically the UCRT headers that ship with it). The link points to the right download page but the name is misleading.

  2. Workaround is too aggressive: Telling users to uninstall other SDK versions is likely to break other tooling. A better approach would be to document how to point clang at the correct SDK (e.g., setting environment variables, or using --sysroot/-isystem flags), or to fix the Makefile to handle this.

  3. Writing style: The surrounding README is terse and direct. The added text is verbose by comparison ("It's important to check whether the work/test machine has installed with any of the Visual Studio SDK versions..."). Please tighten it up to match the existing tone.

  4. Suggested direction: A concise note like "You'll need the Windows SDK installed for UCRT headers. If clang fails to find headers and you have multiple SDK versions under C:\Program Files (x86)\Windows Kits\10\Include, set the INCLUDE environment variable to point at the correct version." would be more useful and less risky than the current text.

Please update based on this review.

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.

2 participants