search

tianocore / edk2

EDK II
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
Other
4.52k stars 2.44k forks source link

BaseTools: fix spec keyword parsing #5907

Open Ashwin-Veeraiah opened 1 month ago

Ashwin-Veeraiah commented 1 month ago

Fixes SPEC keyword parsing to match EDK2 specification

Description

When SPEC keyword is used in the INF file, it does not create the equivalent #define in the autogen.h file of the module. Additionally it creates a line in autogenerated makefile. This generated line will cause builds to break of we follow the specification as defined in the example here, [https://github.com/tianocore-docs/edk2-InfSpecification/blob/master/3_edk_ii_inf_file_format/34_%5Bdefines%5D_section.md] Ex : SPEC USB_SPECIFICATION_VERSION = 2.0 The changes fixes the autogen code to function as mentioned by the documentation.

How This Was Tested

Tested locally with builds.

Integration Instructions

N/A

Ashwin-Veeraiah commented 1 month ago

@bcran @lgao4 Reaching out through a comment, Not able to add anyone as reviewers.

bcran commented 1 month ago

This change looks good, but I'd prefer to have a review from @lgao4 before merging it.

lgao4 commented 1 month ago

SPEC

Can you give one test case?

Ashwin-Veeraiah commented 1 month ago

SPEC

Can you give one test case?

Test case in the code like in the comments? Or in an INF file? Is there a dummy file that I can drop it in for you? ie: The current fix is to match what the EDK2 documentations says it should be,

INF files, Defines Section, Add this SPEC USB_SPECIFICATION_VERSION = 2.0 And you should see it in the autogen.h file, #DEFINE USB_SPECIFICATION_VERSION 2.0, where it is a double type.

Ideally, I would like for it to be more modern be free form (instead of double, make it a string value so I can use multi decimal input). That would require me to change edk2 specification document so thought to try to fix it first before I proposed that. ex : SPEC USB_SPECIFICATION_VERSION = 2.1.2 autogen.h will have "#DEFINE USB_SPECIFICATION_VERSION "2.1.2"

lgao4 commented 1 month ago

SPEC

Can you give one test case?

Test case in the code like in the comments? Or in an INF file? Is there a dummy file that I can drop it in for you? ie: The current fix is to match what the EDK2 documentations says it should be,

INF files, Defines Section, Add this SPEC USB_SPECIFICATION_VERSION = 2.0 And you should see it in the autogen.h file, #DEFINE USB_SPECIFICATION_VERSION 2.0, where it is a double type.

Ideally, I would like for it to be more modern be free form (instead of double, make it a string value so I can use multi decimal input). That would require me to change edk2 specification document so thought to try to fix it first before I proposed that. ex : SPEC USB_SPECIFICATION_VERSION = 2.1.2 autogen.h will have "#DEFINE USB_SPECIFICATION_VERSION "2.1.2"

Edk2 doesn't use double type. So, I agree to define them as ascii string. Please submit the spec change first.

Ashwin-Veeraiah commented 1 month ago

SPEC

Can you give one test case?

Test case in the code like in the comments? Or in an INF file? Is there a dummy file that I can drop it in for you? ie: The current fix is to match what the EDK2 documentations says it should be, INF files, Defines Section, Add this SPEC USB_SPECIFICATION_VERSION = 2.0 And you should see it in the autogen.h file, #DEFINE USB_SPECIFICATION_VERSION 2.0, where it is a double type. Ideally, I would like for it to be more modern be free form (instead of double, make it a string value so I can use multi decimal input). That would require me to change edk2 specification document so thought to try to fix it first before I proposed that. ex : SPEC USB_SPECIFICATION_VERSION = 2.1.2 autogen.h will have "#DEFINE USB_SPECIFICATION_VERSION "2.1.2"

Edk2 doesn't use double type. So, I agree to define them as ascii string. Please submit the spec change first.

Please advise how/steps to follow to request the change to specification please? Do I make a PR against the document (*.MD) files? Do I raise it in the CI/Basetools meetings as a topic?

lgao4 commented 3 weeks ago

SPEC

Can you give one test case?

Test case in the code like in the comments? Or in an INF file? Is there a dummy file that I can drop it in for you? ie: The current fix is to match what the EDK2 documentations says it should be, INF files, Defines Section, Add this SPEC USB_SPECIFICATION_VERSION = 2.0 And you should see it in the autogen.h file, #DEFINE USB_SPECIFICATION_VERSION 2.0, where it is a double type. Ideally, I would like for it to be more modern be free form (instead of double, make it a string value so I can use multi decimal input). That would require me to change edk2 specification document so thought to try to fix it first before I proposed that. ex : SPEC USB_SPECIFICATION_VERSION = 2.1.2 autogen.h will have "#DEFINE USB_SPECIFICATION_VERSION "2.1.2"

Edk2 doesn't use double type. So, I agree to define them as ascii string. Please submit the spec change first.

Please advise how/steps to follow to request the change to specification please? Do I make a PR against the document (*.MD) files? Do I raise it in the CI/Basetools meetings as a topic?

For the change in spec, please send the patch to edk2 mail list devel@edk2.groups.io. I will review and merge it.