Skip to content

[CI-NO-BUILD] [vioscsi] Raise NTDDI version#1309

Draft
benyamin-codez wants to merge 1 commit intovirtio-win:masterfrom
benyamin-codez:vioscsi-raise-ntddi
Draft

[CI-NO-BUILD] [vioscsi] Raise NTDDI version#1309
benyamin-codez wants to merge 1 commit intovirtio-win:masterfrom
benyamin-codez:vioscsi-raise-ntddi

Conversation

@benyamin-codez
Copy link
Copy Markdown
Contributor

@benyamin-codez benyamin-codez commented Feb 21, 2025 •
edited
Loading

Background:
The NTDDI_VERSION has not historically been set in the vioscsi.vcxproj file as PreprocessorDefinitions. Both the Win10 Release and Win11 Release targets are presently compiling with NTDDI_VERSION=NTDDI_WINTHRESHOLD, which is equivalent to Windows 10, version 1507, build 10240, dev codename Threshold 1.

This PR refactors vioscsi.vcxproj to specify:

  1. The preferred NTDDI_VERSION as PreprocessorDefinitions, where we are using:
    (a) NTDDI_WINTHRESHOLD (equiv. Windows 10, version 1507, build 10240, dev codename Threshold 1) for Win10 Release targets; and
    (b) NTDDI_WIN10_CO (equiv. Windows 10/11, version 21H2, build 19044/22000, dev codename Cobalt) for Win11 Release targets.

@benyamin-codez
Copy link
Copy Markdown
Contributor Author

benyamin-codez commented Feb 21, 2025

Waiting on PR #1228 to merge before rebasing and review.

This was referenced Feb 21, 2025
Closed
Draft
kostyanf14
kostyanf14 reviewed Feb 21, 2025
View reviewed changes
Comment thread vioscsi/vioscsi.vcxproj
Comment on lines -176 to +204
<WppEnabled Condition="'$(Configuration)|$(Platform)'=='Win10 Release|x64'">true</WppEnabled>
<WppEnabled Condition="'$(Configuration)|$(Platform)'=='Win11 Release|x64'">true</WppEnabled>
<WppEnabled Condition="'$(Configuration)|$(Platform)'=='Win10 Release|x64' And '$(DisableTracing)'!='true' And '$(UseDebugTracing)'!='true'">true</WppEnabled>
<WppEnabled Condition="'$(Configuration)|$(Platform)'=='Win11 Release|x64' And '$(DisableTracing)'!='true' And '$(UseDebugTracing)'!='true'">true</WppEnabled>
<WppGenerateUsingTemplateFile Condition="'$(Configuration)|$(Platform)'=='Win10 Release|x64'">{km-StorDefault.tpl}*.tmh</WppGenerateUsingTemplateFile>
<WppGenerateUsingTemplateFile Condition="'$(Configuration)|$(Platform)'=='Win11 Release|x64'">{km-StorDefault.tpl}*.tmh</WppGenerateUsingTemplateFile>
<WppScanConfigurationData Condition="'$(Configuration)|$(Platform)'=='Win10 Release|x64'">trace.h</WppScanConfigurationData>
<WppScanConfigurationData Condition="'$(Configuration)|$(Platform)'=='Win11 Release|x64'">trace.h</WppScanConfigurationData>
<WppScanConfigurationData Condition="'$(Configuration)|$(Platform)'=='Win10 Release|x64' And '$(LimitPathsWPP)'=='WPPColdPathOnly'">wpp_cold_path.h</WppScanConfigurationData>
<WppScanConfigurationData Condition="'$(Configuration)|$(Platform)'=='Win10 Release|x64' And '$(LimitPathsWPP)'!='WPPColdPathOnly'">wpp_all_paths.h</WppScanConfigurationData>
<WppScanConfigurationData Condition="'$(Configuration)|$(Platform)'=='Win11 Release|x64' And '$(LimitPathsWPP)'=='WPPColdPathOnly'">wpp_cold_path.h</WppScanConfigurationData>
<WppScanConfigurationData Condition="'$(Configuration)|$(Platform)'=='Win11 Release|x64' And '$(LimitPathsWPP)'!='WPPColdPathOnly'">wpp_all_paths.h</WppScanConfigurationData>
Copy link
Copy Markdown
Member

@kostyanf14 kostyanf14 Feb 21, 2025

Choose a reason for hiding this comment

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

Looks like you merged part of other PR into this commit.

Copy link
Copy Markdown
Contributor Author

@benyamin-codez benyamin-codez Feb 21, 2025

Choose a reason for hiding this comment

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

Yeah, I hope it will merge with PR #1228 and then vanish when I rebase... 8^d

Comment thread vioscsi/vioscsi.vcxproj
<PreprocessorDefinitions Condition="'$(DisableTracing)'=='true'">%(PreprocessorDefinitions);RUN_UNCHECKED=1</PreprocessorDefinitions>
<PreprocessorDefinitions Condition="'$(LimitPathsWPP)'=='WPPColdPathOnly'">%(PreprocessorDefinitions);RUN_COLD_PATH_ONLY=1</PreprocessorDefinitions>
<PreprocessorDefinitions Condition="'$(UseDebugTracing)'!='true'">%(PreprocessorDefinitions);EVENT_TRACING=1</PreprocessorDefinitions>
<PreprocessorDefinitions>%(PreprocessorDefinitions);NTDDI_VERSION=NTDDI_WIN10_CO</PreprocessorDefinitions>
Copy link
Copy Markdown
Member

@kostyanf14 kostyanf14 Feb 21, 2025

Choose a reason for hiding this comment

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

Please write in the commit message NTDDI_WINTHRESHOLD and NTDDI_WIN10_CO with a human-readable version name like Windows 11 21H2

Copy link
Copy Markdown
Contributor Author

@benyamin-codez benyamin-codez Feb 21, 2025

Choose a reason for hiding this comment

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

Just to be clear, they are actual NTDDI versions.
I've amended the commit message with some descriptions and pushed it.
I trust that change is sufficient... 8^d

Copy link
Copy Markdown
Contributor Author

@benyamin-codez benyamin-codez Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also updated the OP with commit message change.

Comment thread vioscsi/vioscsi.vcxproj
Comment on lines +135 to +137
<PreprocessorDefinitions Condition="'$(DisableTracing)'=='true'">%(PreprocessorDefinitions);RUN_UNCHECKED=1</PreprocessorDefinitions>
<PreprocessorDefinitions Condition="'$(LimitPathsWPP)'=='WPPColdPathOnly'">%(PreprocessorDefinitions);RUN_COLD_PATH_ONLY=1</PreprocessorDefinitions>
<PreprocessorDefinitions Condition="'$(UseDebugTracing)'!='true'">%(PreprocessorDefinitions);EVENT_TRACING=1</PreprocessorDefinitions>
Copy link
Copy Markdown
Member

@kostyanf14 kostyanf14 Feb 21, 2025

Choose a reason for hiding this comment

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

Looks like you merged part of other PR into this commit.

Copy link
Copy Markdown
Contributor Author

@benyamin-codez benyamin-codez Feb 21, 2025

Choose a reason for hiding this comment

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

Yeah, I hope it will merge with PR #1228 and then vanish when I rebase... 8^d

@benyamin-codez
Copy link
Copy Markdown
Contributor Author

benyamin-codez commented Feb 21, 2025 •
edited
Loading

Looks like you merged part of other PR into this commit.

Yeah, I hope it will merge with PR #1228 and then vanish when I rebase... 8^d

I could clean it up this PR and let it merge first and then rebase the other PR...
...but I haven't even raised PR(s) dependent on this one yet (which would be my motivation).
Having said that, if your team or the red fedoras would prefer it, I can certainly do that.
Otherwise, I'm happy to progress the other PR first. Let me know.

@benyamin-codez
[vioscsi] Raise NTDDI version
e97d8ac 
Background:
The NTDDI_VERSION has not historically been set in the vioscsi.vcxproj file
as PreprocessorDefinitions. Both the Win10 Release and Win11 Release targets
are presently compiling with NTDDI_VERSION=NTDDI_WINTHRESHOLD, which is
equivalent to Windows 10, version 1507, build 10240, dev codename Threshold 1.

This PR refactors vioscsi.vcxproj to specify:

1. The preferred NTDDI_VERSION as PreprocessorDefinitions, where we are using
   (a) NTDDI_WINTHRESHOLD (equiv. Windows 10, version 1507, build 10240,
       dev codename Threshold 1) for Win10 Release targets; and
   (b) NTDDI_WIN10_CO (equiv. Windows 10/11, version 21H2, build 19044/22000
       dev codename Cobalt) for Win11 Release targets.

Signed-off-by: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com>
@benyamin-codez
Copy link
Copy Markdown
Contributor Author

benyamin-codez commented Feb 21, 2025

Also should point out I had previously asked:

Does this need to be done by a different means anyway, e.g. via *.rc or *.ver files?

@benyamin-codez benyamin-codez mentioned this pull request Feb 26, 2025
Draft
@YanVugenfirer
Copy link
Copy Markdown
Collaborator

YanVugenfirer commented Aug 15, 2025

[Jenkins CI]: Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kostyanf14 kostyanf14 kostyanf14 left review comments

@vrozenfe vrozenfe Awaiting requested review from vrozenfe vrozenfe will be requested when the pull request is marked ready for review vrozenfe is a code owner

@YanVugenfirer YanVugenfirer Awaiting requested review from YanVugenfirer YanVugenfirer will be requested when the pull request is marked ready for review YanVugenfirer is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@benyamin-codez @YanVugenfirer @kostyanf14