Skyline: Improve code for finding Thermo method export DLLs #3352

brendanx67 · 2025-01-30T23:32:23Z

Thermo reported this code as failing on an Ascend at Thermo
This code was written after a full review of 7 machines in the MacCoss lab and the Ascend at Thermo to do a more complete job of finding the necessary DLLs
More recent machines have added Thermo.TNG.MethodXMLInterface2.dll to support Adaptive RT so it needs to be copied if it is found
Added a fairly thorough unit test for this more complicated new code to make sure it works as expected and stays working

- Thermo reported this code as failing on an Ascend at Thermo - This code was written after a full review of 7 machines in the MacCoss lab and the Ascend at Thermo to do a more complete job of finding the necessary DLLs - More recent machines have added Thermo.TNG.MethodXMLInterface2.dll to support Adaptive RT so it needs to be copied if it is found - Added a fairly thorough unit test for this more complicated new code to make sure it works as expected and stays working

- but also turn off DLL copying, since Thermo feels this should not be necessary and the variety of DLLs we are seeing is not their intention. - easier to test DLL combinations when this code is turned off

- made it possible to have just "Thermo" as a method type where Skyline auto-detects the instrument form the registry

…t a valid installation

- expanded testing to include command-line

…ProteoWizard/pwiz into Skyline/work/20250130_thermo_dll_finder

- added error message testing in the ExportMethodDlg

brendanx67 · 2025-03-07T04:11:47Z

I think this is ready to go. I will do another round of live testing tomorrow in the lab, but I am not expecting any surprises. It is pretty well tested. Would appreciate at least one code review before merging.

nickshulman · 2025-03-07T04:40:20Z

On my desktop computer my registry looks like this:

The ProgramPath is the relative path "System\Programs\dependencies\tng" which actually seems to refer to the path "C:\Program Files\Thermo Scientific\Instruments\Astral\1.1\System\Programs\dependencies\tng".

There was a support request from a user last year who also had a relative path in the registry:
https://skyline.ms/announcements/home/support/thread.view?rowId=63895

They apparently were able to fix their problem by changing the relative path in the registry to an absolute path.

The error message I got was:

Does Skyline have enough information to figure out how to turn the relative path into an absolute path?
If it were to look in the correct absolute path it would find the dll's it's looking for.

brendanx67 · 2025-03-07T05:06:23Z

What is the full path in this case? Pretty explicit error message, though. If it were posted on the support board, we would know what to tell the user to do.

…

On Thu, Mar 6, 2025 at 8:40 PM nickshulman ***@***.***> wrote: On my desktop computer my registry looks like this: image.png (view on web) <https://github.com/user-attachments/assets/f7334c9e-7923-4441-911d-541f6e2fac5b> The ProgramPath is the relative path "System\Programs\dependencies\tng" which actually seems to refer to the path "C:\Program Files\Thermo Scientific\Instruments\Astral\1.1\System\Programs\dependencies\tng". There was a support request from a user last year who also had a relative path in the registry: https://skyline.ms/announcements/home/support/thread.view?rowId=63895 They apparently were able to fix their problem by changing the relative path in the registry to an absolute path. The error message I got was: image.png (view on web) <https://github.com/user-attachments/assets/b47b76f8-fbf4-4d26-99b2-024cffa6cb68> Does Skyline have enough information to figure out how to turn the relative path into an absolute path? If it were to look in the correct absolute path it would find the dll's it's looking for. — Reply to this email directly, view it on GitHub <#3352 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYBWUGM3I2CLJZW27NGVJD2TEPMTAVCNFSM6AAAAABWGMV7KKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBVGUYTENBSGU> . You are receiving this because you authored the thread.Message ID: ***@***.***> [image: nickshulman]*nickshulman* left a comment (ProteoWizard/pwiz#3352) <#3352 (comment)> On my desktop computer my registry looks like this: image.png (view on web) <https://github.com/user-attachments/assets/f7334c9e-7923-4441-911d-541f6e2fac5b> The ProgramPath is the relative path "System\Programs\dependencies\tng" which actually seems to refer to the path "C:\Program Files\Thermo Scientific\Instruments\Astral\1.1\System\Programs\dependencies\tng". There was a support request from a user last year who also had a relative path in the registry: https://skyline.ms/announcements/home/support/thread.view?rowId=63895 They apparently were able to fix their problem by changing the relative path in the registry to an absolute path. The error message I got was: image.png (view on web) <https://github.com/user-attachments/assets/b47b76f8-fbf4-4d26-99b2-024cffa6cb68> Does Skyline have enough information to figure out how to turn the relative path into an absolute path? If it were to look in the correct absolute path it would find the dll's it's looking for. — Reply to this email directly, view it on GitHub <#3352 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYBWUGM3I2CLJZW27NGVJD2TEPMTAVCNFSM6AAAAABWGMV7KKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBVGUYTENBSGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

brendanx67 · 2025-03-07T15:37:10Z

I don't think we are going to fix that problem in this PR. I like that the error message is pretty clear for it. None of the instruments in the MacCoss lab have this issue. If you can reproduce how you ended up with your registry looking like this, we should report it to Thermo, but is may already be fixed on their end.

nickshulman · 2025-03-07T18:37:20Z

I don't think we are going to fix that problem in this PR.

Sounds good. I could not find anything in my registry that could be used to figure out that "C:\Program Files\Thermo Scientific\Instruments\Astral\1.1"

brendanx67 · 2025-03-07T21:05:45Z

Any other thoughts on this PR before I merge to master?

nickshulman

Looks good to me.

nickshulman · 2025-03-07T21:21:06Z

pwiz_tools/Skyline/Model/DllFinder.cs

+        private class ProgramPath
+        {
+            public string Path;
+            public bool IsFull;


This probably should be called "IsFullInstall" because otherwise it sure sounds like it means "IsFullPath" given the class it's in.

Okay. Changed to IsFullInstall and added comments.

nickshulman · 2025-03-07T21:27:36Z

pwiz_tools/Skyline/FileUI/ExportMethodDlg.cs

+        {
+            string selectionType = GetSelectedInstrument(instrumentType);
+            if (listTypes.Contains(selectionType) && GetInstrumentTypeFromSelection(selectionType, true) != null)
+                InstrumentType = listTypes.FirstOrDefault(typeName => typeName.Equals(instrumentType));


This code is a little confusing, because there's "instrumentType", "selectionType" and whatever "GetInstrumentTypeFromSelection(selectionType)" returns.

This particular line is equivalent to:
InstrumentType = listTypes.Contains(instrumentType) ? instrumentType : null;

Is that what you intended? (it probably is)
Just wanted to make sure this code is doing what you expect it to.

Okay. Good point. I could barely understand it myself, and I am not sure it worked correctly. I have added a bunch of comments, and several tests of the initial selection of the Instrument type combo box, and also improved the code to work exactly the way I was expecting it to, after debug stepping through it during the tests.

…nitial selection of Instrument type combo box

…ProteoWizard/pwiz into Skyline/work/20250130_thermo_dll_finder

…tion

brendanx67 requested review from nickshulman and rita-gwen January 30, 2025 23:32

brendanx67 and others added 13 commits January 31, 2025 07:22

- added tests of DLL overwrite based on DLL last modified dates

49ecf64

- improved support for cases seen in the MacCoss lab

c146d55

- but also turn off DLL copying, since Thermo feels this should not be necessary and the variety of DLLs we are seeing is not their intention. - easier to test DLL combinations when this code is turned off

- fixes to Thermo method export

ff9e0a3

- made it possible to have just "Thermo" as a method type where Skyline auto-detects the instrument form the registry

- fixes to ExportMethodDlg based on more testing on a computer withou…

59e9858

…t a valid installation

Merge branch 'master' into Skyline/work/20250130_thermo_dll_finder

34ca4e9

- fixed command-line --exp-method-instrument to work with "Thermo"

21dac0b

- expanded testing to include command-line

Merge branch 'Skyline/work/20250130_thermo_dll_finder' of github.com:…

63d8118

…ProteoWizard/pwiz into Skyline/work/20250130_thermo_dll_finder

- improved error messages for failure to find Thermo installation

e330b1d

Merge branch 'master' into Skyline/work/20250130_thermo_dll_finder

9edc60f

- add missing files and re-org DllFinder testing classes into TestUtil

298af7d

Merge branch 'Skyline/work/20250130_thermo_dll_finder' of github.com:…

32f2cfb

…ProteoWizard/pwiz into Skyline/work/20250130_thermo_dll_finder

- fix unused using clause

ca269fd

- added error message testing in the ExportMethodDlg

- remove code accidentally left in with last commit

1427d49

Merge branch 'master' into Skyline/work/20250130_thermo_dll_finder

0391861

nickshulman approved these changes Mar 7, 2025

View reviewed changes

brendanx67 added 3 commits March 7, 2025 15:01

- responding to Nick's review more commenting and better testing of i…

e4192fa

…nitial selection of Instrument type combo box

Merge branch 'Skyline/work/20250130_thermo_dll_finder' of github.com:…

454d224

…ProteoWizard/pwiz into Skyline/work/20250130_thermo_dll_finder

- better naming and commenting of ProgramPath class per Nick's sugges…

c66ec77

…tion

brendanx67 merged commit 73af77a into master Mar 8, 2025
11 checks passed

brendanx67 deleted the Skyline/work/20250130_thermo_dll_finder branch March 8, 2025 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skyline: Improve code for finding Thermo method export DLLs #3352

Skyline: Improve code for finding Thermo method export DLLs #3352

brendanx67 commented Jan 30, 2025

brendanx67 commented Mar 7, 2025

nickshulman commented Mar 7, 2025

brendanx67 commented Mar 7, 2025 via email

brendanx67 commented Mar 7, 2025

nickshulman commented Mar 7, 2025 •

edited

Loading

brendanx67 commented Mar 7, 2025

nickshulman left a comment

nickshulman Mar 7, 2025

brendanx67 Mar 7, 2025

nickshulman Mar 7, 2025

brendanx67 Mar 7, 2025

Skyline: Improve code for finding Thermo method export DLLs #3352

Skyline: Improve code for finding Thermo method export DLLs #3352

Conversation

brendanx67 commented Jan 30, 2025

brendanx67 commented Mar 7, 2025

nickshulman commented Mar 7, 2025

brendanx67 commented Mar 7, 2025 via email

brendanx67 commented Mar 7, 2025

nickshulman commented Mar 7, 2025 • edited Loading

brendanx67 commented Mar 7, 2025

nickshulman left a comment

Choose a reason for hiding this comment

nickshulman Mar 7, 2025

Choose a reason for hiding this comment

brendanx67 Mar 7, 2025

Choose a reason for hiding this comment

nickshulman Mar 7, 2025

Choose a reason for hiding this comment

brendanx67 Mar 7, 2025

Choose a reason for hiding this comment

nickshulman commented Mar 7, 2025 •

edited

Loading