Cleanup: move NTLM status from defines to enum

[kinkie]

NTLM status codes are defines and overlap somewhat. Turn them into a C++ enum class.

[rousskov]

I polished PR title for consistency sake and polished PR description punctuation a bit. Please adjust further as needed.

[kinkie]

I'm looking into setting up a mingw-based crosscompiler; let's see if it can unrot the windows-specific code. @rousskov , if you agree I'd defer unrotting this helper to the windows specific effort

[rousskov]

if you agree I'd defer unrotting this helper to the windows specific effort

Sorry, I am not sure what that means for this PR. Please clarify; for example, are you proposing that this PR

1. knowingly damages some Windows-specific code further (because that code is known to be broken already)?
2. knowingly breaks some Windows-specific code (because you cannot easily test that code)?
3. modifies some Windows-specific code without testing it (because you cannot easily test that code)?
4. is put aside until you can test the affected Windows-specific code (so that you do not make things worse for Windows folks)?
5. does something else?

Please note that none of the above descriptions are meant to sound negative. I am just trying to understand what you want to do (and why) before I can answer your question in a meaningful way.

Needless to say, it would be nice to know what support tier we place Windows OS in, and what that tier means in terms of code breakage and PR testing, but I suspect we will not settle that issue by the time you need to get the answer to your question.

[kinkie]

On Wed, 24 Jun 2020 at 23:08, Alex Rousskov [email protected] wrote:

if you agree I'd defer unrotting this helper to the windows specific effort

Sorry, I am not sure what that means for this PR. Please clarify; for example, are you proposing that this PR

1. knowingly damages some Windows-specific code further (because that code is known to be broken already)?
2. knowingly breaks some Windows-specific code (because you cannot easily test that code)?
3. modifies some Windows-specific code without testing it (because you cannot easily test that code)?
4. is put aside until you can test the affected Windows-specific code (so that you do not make things worse for Windows folks)?
5. does something else?

The most similar is 4. The windows-only code is already broken, so these changes to the windows-only ntlm-sspi helper can’t be tested. I’d like to leave it broken for a short while, to avoid scope creep.

Please note that none of the above descriptions are meant to sound negative. I am just trying to understand what you want to do (and why) before I can answer your question in a meaningful way.

Needless to say, it would be nice to know what support tier we place Windows OS in, and what that tier means in terms of code breakage and PR testing, but I suspect we will not settle that issue by the time you need to get the answer to your question.

I’m looking into adding mingw cross compiling on Linux for a windows target. If that works it will integrate nearly in our build environment. Next option is to have a windows build host, I’m looking into this using the RackSpace donation.

Another option would be GitHub actions but that’s quite indirect so I’m not sure if it’s worth the effort right now for this specific use case

@mobile

[yadij]

FYI: Rafael at Diladele is on a full lockdown until COVID-19 was resolved in NL and so only supporting v3.5 for now due to some build issues with v4+. Once the lockdown is over I am hoping he can provide us access to a docker image we can integrate to the farm and collaborate better on build issues.

[kinkie]

That would be great! In the meantime, are you supporting my proposal or would you prefer a different approach?

@mobile

[yadij]

Since we know there are uncertain bugs in this (Windows-specific) code and unable currently to test it I would prefer not to be touching it at all. If this was triggered by your MinGW tests and found these defines to be a bug, that is fine. But cleanup just to remove a TODO is IMO needing ability to at least build test to accept - otherwise it may not actually be a fix.

[rousskov]

Alex: are you proposing that this PR ... (4) is put aside until you can test the affected Windows-specific code (so that you do not make things worse for Windows folks)?

Francesco: The most similar is 4. The windows-only code is already broken, so these changes to the windows-only ntlm-sspi helper can’t be tested. I’d like to leave it broken for a short while, to avoid scope creep.

I fully support option 4 -- setting this PR aside until we can test it. Unfortunately, I do not think that is how you interpret that option, so I still do not know (or are not sure at all) what you are proposing. FWIW, "break the already broken code further" is option 1. There are many valid ways to proceed here -- I am not implying that option 4 is the only such option.

to avoid scope creep

I see no scope creep. IMO: If we want to replace a group of related names used in Squid code, we should replace those names everywhere. If we touch tier-1 Squid code, we should test those changes before officially merging. If we cannot test tier-1 code, we should not polish it. Unfortunately, I do not know Windows' tier, and I am not sure there is consensus regarding these rules.

[kinkie]

Suspending this PR until we have a windows test host to hash out issues on

[rousskov]

FTR: I replaced the S-waiting-for-windows-testbed label text with S-waiting-for-QA to slow down label proliferation and to continue naming a specific actor (to the extent possible) as the party responsible for ending the wait. I also changed the label color to match the rest of the S-waiting-* group.

[rousskov]

Alex, a year ago: FTR: I replaced the S-waiting-for-windows-testbed label text with S-waiting-for-QA to slow down label proliferation and to continue naming a specific actor (to the extent possible) as the party responsible for ending the wait. I also changed the label color to match the rest of the S-waiting-* group.

kinkie added the waiting-for-windows-qa label 3 minutes ago

We can add platform labels if really needed, but I would not add a platform indication as other label suffixes to avoid label explosion. Once we cross a certain number of labels, they become very difficult for humans to use... I have now removed the waiting-for-windows-qa label. If you decide to add platform label(s), please start/follow a common pattern (e.g., P-* for platform or perhaps a more general E-* for environment) to ease grouping of labels and label manipulation.

FWIW, our labeling goal should not be to describe the exact PR state, down to minute details (which would eventually require dealing with 1000 labels or 100-word labels). The goal should be to quickly tell the reader whether they should work on this PR (now) or skip it. The low-level details should be in the PR itself.

[kinkie]

I added that to mark to myself that that PR is on a hiatus, as rightly highlighted by Amos, we shouldn't touch what we can't test. I'm hopefully going to have access to a windows machine soon, that should help

On Fri, May 21, 2021 at 9:24 PM Alex Rousskov @.***> wrote:

Alex, a year ago https://github.com/squid-cache/squid/pull/676#issuecomment-652409016: FTR: I replaced the S-waiting-for-windows-testbed label text with S-waiting-for-QA to slow down label proliferation and to continue naming a specific actor (to the extent possible) as the party responsible for ending the wait. I also changed the label color to match the rest of the S-waiting-* group.

kinkie added the waiting-for-windows-qa label 3 minutes ago

We can add platform labels if really needed, but I would not add a platform indication as other label suffixes to avoid label explosion. Once we cross a certain number of labels, they become very difficult for humans to use... I have now removed waiting-for-windows-qa label. If you decide to add platform label(s), please start/follow a common pattern like (P-*) to ease grouping of labels and label manipulation.

FWIW, our labeling goal should not be to describe the exact PR state, down to minute details (which would eventually require dealing with 1000 labels or 100-word labels). The goal should be to quickly tell the reader whether they should work on this PR (now) or skip it. The low-level details should be in the PR itself.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/squid-cache/squid/pull/676#issuecomment-846193172, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHPVDGOFTFCRJI3ZXH64KTTO2XOXANCNFSM4OD2A3SA .

-- Francesco

[rousskov]

I added that to mark to myself that that PR is on a hiatus

Sure, but we have to coordinate when it comes to Project-wide things such as PR labels. Unfortunately, GitHub does not support private labels.

FWIW, I recommend removing S-waiting-for-author if you are not going to work on this until we get a Windows machine - this PR is not really waiting on your action, it is waiting on QA infrastructure improvements, which S-waiting-for-QA already indicates. Without S-waiting-for-author, this PR will not be on your personal TODO list (if you compute that list using labels) and others will not erroneously think that this PR is waiting for your action.

And again, if you are sure that labeling a dominating or blocking PR platform/environment is very useful, then we should add a separate set of labels for that as well. My worry is that, in my experience, the more labeling categories one gets, the more difficult it becomes to label the PR correctly, and the fewer PRs will have correct/reliable labeling, defeating the primary purpose of the labels and making all labels a lot less useful.

[yadij]

Aye, simpler is better. For S-waiting-* we go by group of stakeholders. With a COMMENT in the PR thread when unusual ones are set to indicate why.

[kinkie]

I have now set up a Windows VM to test things, but not yet the tooling. I'll be able to unblock this soon

[kinkie]

I've managed to successfully build the helper in mingw. This should cover the requirement for QA:

$ do-build --parallel 10 --do-build --dir btbuild --mingw --continue --clean-before
[...]
$ ls -l btbuild/src/auth/ntlm/*/*.exe
-rwxr-xr-x 1 kinkie kinkie 58368 Dec 25 12:24 btbuild/src/auth/ntlm/SSPI/ntlm_sspi_auth.exe

[kinkie]

I’ll hand check it and attach evidence, this code is not built as part of the normal procedures

@mobile

On Sat, 30 Dec 2023 at 13:13, Amos Jeffries @.***> wrote:

@.**** approved this pull request.

LGTM, provided it builds after this latest merge.

— Reply to this email directly, view it on GitHub https://github.com/squid-cache/squid/pull/676#pullrequestreview-1799553046, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHPVDFLQNF6V5NPMMLTRM3YMAAOPAVCNFSM4OD2A3SKU5DIOJSWCZC7NNSXTPCQOVWGYUTFOF2WK43UKJSXM2LFO45TCNZZHE2TKMZQGQ3A . You are receiving this because you authored the thread.Message ID: @.***>

[kinkie]

Build evidence: $ do-build --parallel 10 --do-build --dir btbuild --log ../build.log --mingw --continue --clean-before [...] $ file btbuild/src/auth///*.exe btbuild/src/auth/basic/fake/basic_fake_auth.exe: PE32+ executable (console) x86-64 (stripped to external PDB), for MS Windows btbuild/src/auth/basic/SSPI/basic_sspi_auth.exe: PE32+ executable (console) x86-64 (stripped to external PDB), for MS Windows btbuild/src/auth/ntlm/SSPI/ntlm_sspi_auth.exe: PE32+ executable (console) x86-64 (stripped to external PDB), for MS Windows

[kinkie]

@yadij , merging from master has voided your previous approval; could you re-approve?

[kinkie]

ok to test

[kinkie]

@rousskov do you mind stamping this PR? As you have been requested to review, it can't move forward

[rousskov]

@rousskov do you mind stamping this PR? As you have been requested to review, it can't move forward

My status has not changed since https://github.com/squid-cache/squid/pull/676#pullrequestreview-1796024275: I cannot invest more time into this Windows-specific PR. I am declining this new opportunity to review it under the old assumption that my earlier (grave) concerns have been addressed.

[kinkie]

@rousskov do you mind stamping this PR? As you have been requested to review, it can't move forward

My status has not changed since #676 (review): I cannot invest more time into this Windows-specific PR. I am declining this new opportunity to review it under the old assumption that my earlier (grave) concerns have been addressed.

that's okay, I'm not asking that you endorse code you can't test. Github was showing that you had been requested to review and pending that was not allowing the merge to proceed. That has now changed and the reviews by myself and Amos can allow the process to continue