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

BaseTools: Allow ascii exceptions for ECC #5867

Closed

Conversation

leiflindholm
Copy link
Member

As part of trying to merge #5840,
I realised ECC was not respecting the new exception I added to EmbeddedPkg
in order to pass CI.

So I did some digging in ECC, and first of all found "ascii" was consistently
misspelt throughout, so first a patch to fix that.

But then I also found it bails if it finds any non-7-bit characters without
even looking at the exceptions. So I added support for checking the exception
list. As part of that, the error reporting drops to one report per line.

I don't tend to write much python, and I'm sure someone who does could do it
more cleanly. Feedback very welcome.

@mariobalanica @mdkinney @makubacki

Ecc concistently referred to ASCII/Ascii as ACSII/Acsii, which
bugged me to no end when trying to figure out how those tests
worked. Fix all instances.

Signed-off-by: Leif Lindholm <[email protected]>
The ECC test for presence of non-ASCII (non-7-bit) characters bailed out
directly on finding a match, without checking the exception list.
This patch implements rudimentary support for respecting exceptions.

As a side effect, this reduces the number of reports to one per affected
line (pointing out all affected positions) instead of one per
character/line combination.

Signed-off-by: Leif Lindholm <[email protected]>
@mdkinney
Copy link
Member

mdkinney commented Jul 9, 2024

If non-ASCII is required in a source file comment, then we need to decide the file format and encodings supported and we need to make sure every tool that may open a source file is compatible with that encoding. I know over the years, we have had both commercial tools and custom tools that would fail if the source file type was not ASCII. Compilers, static analysis, document generators, and statistics collection are just a few examples of the types of tools that open/read source files. We also have to make sure all the editors that the community is using supports the select file type/encodings.

@leiflindholm
Copy link
Member Author

@mdkinney I added a response in a new discussion at #5893.

Now, fundamentally, what I really want is an override label that lets a maintainer override ECC when it's being unhelpful (like in #5840) and blocking a submission from a new contributor from being merged.

@mdkinney
Copy link
Member

mdkinney commented Jul 9, 2024

Is this still the ASCII check in ECC or a different ECC reported issue that needs to be fixed.

@leiflindholm
Copy link
Member Author

Is this still the ASCII check in ECC or a different ECC reported issue that needs to be fixed.

Still the check in ECC. (Commit 1/2 is still a valid fix without the behavioral change of 2/2.)

@leiflindholm
Copy link
Member Author

I am abandoning this PR in favour of #5898.

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.

None yet

3 participants