gh-131020: Pylauncher does not correctly detect a BOM when searching for the shebang

[chris-eibl]

This is due to

cpython/PC/launcher2.c

Lines 1080 to 1081 in 475f933

	if (bytesRead > 3 && *b == 0xEF) {
	if (*++b == 0xBB && *++b == 0xBF) {

for wich clang-cl creates the following warnings:

1>..\PC\launcher2.c(1080,29): warning : result of comparison of constant 239 with expression of type 'char' is always false [-Wtautological-constant-out-of-range-compare]
1>..\PC\launcher2.c(1081,34): warning : result of comparison of constant 191 with expression of type 'char' is always false [-Wtautological-constant-out-of-range-compare]
1>..\PC\launcher2.c(1081,18): warning : result of comparison of constant 187 with expression of type 'char' is always false [-Wtautological-constant-out-of-range-compare]

The fix is easy. I first created two failing tests.

• Issue: Pylauncher does not correctly detect a BOM when searching for the shebang #131020

[StanFromIreland]

You do not need to update the branch see the devguide.

You do need a NEWS entry :-)

[chris-eibl]

You do not need to update the branch see the devguide.

I updated before I pushed - seems okay to me?

You do need a NEWS entry :-)

Done!

[StanFromIreland]

There is no need to update every time you push. Please read the devguide, the "Update branch button" section (and really the devguide in general, it's got lots of interesting info!).

[chris-eibl]

There is no need to update every time you push. Please read the devguide, the "Update branch button" section (and really the devguide in general, it's got lots of interesting info!).

Yeah, I know. This was just the first push, so I should be fine to update before.
The idea of course is to keep the CI and the notifications low, but that can't apply to the first push - or am I completely mistaken?

[StanFromIreland]

If it's not horrendously behind it's not really necessary.

[picnixz]

Please rewrite the NEWS entry with the correct issue number (the old one). We usually close new duplicates and keep old ones.

[picnixz]

Yeah, I know. This was just the first push, so I should be fine to update before.
The idea of course is to keep the CI and the notifications low, but that can't apply to the first push - or am I completely mistaken?

No it's fine if it's the first push... What we don't want is someone who continuously click on "Update branch" when there is essentially nothing new at all. If you have forked your branch from a quite old main, it's fine to hit the update button once.

[chris-eibl]

Just to reassure: #99620 was about py.ini and pyw.ini.
The test I created was for a shebang in a Python file. Maybe both things go through the same code - let me check.

[picnixz]

They are likely to go through the same according to the issue description but if I have incorrectly closed the issue I'll reopen it

[chris-eibl]

#99620 is not fixed by this PR. I've created test_py_default_with_valid_bom and marked it asexpectedFailure. It is a different code path:

cpython/PC/launcher2.c

Lines 920 to 928 in 475f933

	int
	_readIni(const wchar_t *section, const wchar_t *settingName, wchar_t *buffer, int bufferLength)
	{
	wchar_t iniPath[MAXLEN];
	int n;
	if (SUCCEEDED(SHGetFolderPathW(NULL, CSIDL_LOCAL_APPDATA, NULL, 0, iniPath)) &&
	join(iniPath, MAXLEN, L"py.ini")) {
	debug(L"# Reading from %s for %s/%s\n", iniPath, section, settingName);
	n = GetPrivateProfileStringW(section, settingName, NULL, buffer, bufferLength, iniPath);

Internet search for "GetPrivateProfileStringW" "BOM" seems to reveal that indeed that there are issues with it.

Not sure how to proceed here. These two seem related because they both report problems of the launcher with a BOM. Yet, the other issue deals with py.ini and pyw.ini, but this one is about a BOM in the Python file when searching for the shebang.

Since I do not know how to easily fix the GetPrivateProfileStringW problem, can we treat this as two different issues with different fixes?

I'd update the issue title of #131020 to "Pylauncher does not correctly detect a BOM in the Python file when looking for the shebang" (seems too long, though).

I think the blurb issue and text is then ok?

[chris-eibl]

BTW: looking into the data = self.run_py(["-arg"]) in test_py_default_with_valid_bom reveals the same
# Did not find file C:\\Users\\suc\\AppData\\Local\\py.ini\r\n as mentioned in #99620 (comment), so I think this tests the right thing.

[picnixz]

Oh so it wasn't a duplicate. Ok for reopening and changing the issue number back to what it was then.

[chris-eibl]

I've updated the issue and PR title to be more specific: "when searching for the shebang".

[zooba]

We don't read from the INI file ourselves, and unless the SC rejects PEP 773 we're not going to replace the existing Windows API call with our own code.

I'd just remove this test.

[zooba]

What we don't want is someone who continuously click on "Update branch" when there is essentially nothing new at all.

There shouldn't be any harm in updating the branch, provided you've done a fairly normal branch/PR.

The problem is when you update incorrectly (e.g. merging 3.13 into a PR against main), when you rebase, or when you force push. Provided you just git merge origin/main; git push it's not going to cause any issues or any noise.

[picnixz]

(Most of the time it's to avoid wasting CI resources and notifying those that are subscribed to the conversation. This causes lots of noise since we can't unsubscribe from such commits unfortunately)

[miss-islington-app]

Thanks @chris-eibl for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-131047 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-131048 is a backport of this pull request to the 3.12 branch.

[miss-islington-app]

Sorry @chris-eibl and @zooba, I had trouble completing the backport.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Please backport backport using cherry_picker on the command line.

cherry_picker 36ef3bfe39d767e283b55fe86f34e7671b7f5d1c 3.12