Update WIFF2 SDK to SciexToolkit #270
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in weird application .config files (the DLL now creates those on the fly); unfortunately this update has 2 drawbacks: 1) cannot get SIM targets, and 2) WIFF1 files are not closed properly (a problem which I solved with `pwiz::util::force_close_handles_for_filepath()`); a future SDK update will fix these issues - fixed Analysis.yep file remaining locked after closing MSData * reverted System.Data.SQLite.dll to 1.0.98 to avoid loading multiple versions of SQLite into same AppDomain * edited UIMF DLLs to point at reverted SQLite DLL * fixed WIFF2 files not being included in Reader_ABI_Test * set Skyline to always use msparser.dll (msparserD.dll should only be used with a full C++ debug build) * set AutoGenerateBindingRedirects=false for Skyline project files, although this may not be necessary with everything using the same SQLite version * added pwiz::util::force_close_handles_for_filepath() * fixed some compiler warnings about dtor exceptions * fixed 32-bit build not including TIMS support (I could swear I already did this but can't find it!) * moved RawFile::Lock to cpp_cli_utilities.hpp * changed locked files to be a fatal error in VendorReaderTestHarness (no vendor should be leaving locked files now) * removed VC10 DLLs from WiX installers
chambm
force-pushed
the
feature/wiff2-sdk-update
branch
from
bfc3715
to
43a7967
Compare
* updated WiX templates to SciexToolKit DLLs
* restored SQLite DLLs in subdirectory for SciexToolKit; still not sure whether they're really needed or not, but in any case SciexToolKit does not work on x86 * added GetFileHandleTypeNumber() since file handle types differ between different versions of Windows; it's a heuristic because I haven't found a definitive list of them
…it support and avoid file locking issues (WIFF2 only works on 64-bit though) * disabled WIFF2 unit tests in 32-bit builds * fixed Linux compile
…se I could not get the former to work on Windows Server 2012 (TeamCity) * fixed filename comparison case sensitivity when copying assemblies (was causing SQLite copy errors)
|
@brendanx67 I think this is ready to go. Can it be set as integration branch? The new file locking code turned out to be more useful for Bruker CompassXtract than WIFF, because I ended up needing to use both the old Sciex SDK and the new one to support both WIFF1 and WIFF2. WIFF2 only on 64-bit for now though. When Sciex gets me a newer release of their new SDK hopefully it will have the WIFF1 locking and 32-bit issues fixed. |
…the WIFF2 SciexToolKit API * changed Skyline's WIFF2 test to be conditioned on CultureInfo.NumberFormat (until 2019 when the test will start running unconditionally)
brendanx67
reviewed
Oct 5, 2018
There was a problem hiding this comment.
Couple minor questions. Mostly looks good to me. Let's give it until Monday to prove it passes everything in nightly tests. I will add a couple more machines to test it and get a sense of whether it changes memory use. Does it change our installer size much? Are we adding the WIFF2 libraries without removing WIFF1?
| @@ -170,7 +170,7 @@ PWIZ_API_DECL ChromatogramPtr ChromatogramList_ABI::chromatogram(size_t index, b | |||
| //result->precursor.isolationWindow.set(MS_isolation_window_upper_offset, ie.q1, MS_m_z); | |||
| result->precursor.activation.set(MS_CID); | |||
| result->precursor.activation.set(MS_collision_energy, target.collisionEnergy, UO_electronvolt); | |||
| result->precursor.activation.userParams.push_back(UserParam("MS_declustering_potential", lexical_cast<string>(target.declusteringPotential), "xs:float")); | |||
| //result->precursor.activation.userParams.push_back(UserParam("MS_declustering_potential", lexical_cast<string>(target.declusteringPotential), "xs:float")); | |||
There was a problem hiding this comment.
Do you really mean to comment out declustering potential? This isn't mentioned in the commit notes.
There was a problem hiding this comment.
Yes, it was always being set as 0 IIRC.
nickshulman
approved these changes
Oct 5, 2018
There was a problem hiding this comment.
This looks good to me. I will go ahead and update the Skyline Administrator Installer file list (pwiz_tools\Skyline\Executables\Installer\product.wxs).
By the way, when you say "removed VC10 DLLs from WiX installers" does that mean that there are some .dll's that we can remove from the Skyline setup?
…o avoid the Thread and Event handle leak it causes
chambm
added a commit
that referenced
this pull request
Dec 4, 2018
- updated WIFF2 SDK to SciexToolkit which avoids us needing to maintain weird application .config files (the DLL now creates those on the fly); unfortunately this update has 2 drawbacks: 1) cannot get SIM targets, and 2) WIFF1 files are not closed properly (a problem which I solved with `pwiz::util::force_close_handles_for_filepath()`); a future SDK update will fix these issues - fixed Analysis.yep file remaining locked after closing MSData * reverted System.Data.SQLite.dll to 1.0.98 to avoid loading multiple versions of SQLite into same AppDomain * edited UIMF DLLs to point at reverted SQLite DLL * fixed WIFF2 files not being included in Reader_ABI_Test * set Skyline to always use msparser.dll (msparserD.dll should only be used with a full C++ debug build) * set AutoGenerateBindingRedirects=false for Skyline project files, although this may not be necessary with everything using the same SQLite version * added pwiz::util::force_close_handles_for_filepath() * fixed some compiler warnings about dtor exceptions * fixed 32-bit build not including TIMS support (I could swear I already did this but can't find it!) * moved RawFile::Lock to cpp_cli_utilities.hpp * changed locked files to be a fatal error in VendorReaderTestHarness (no vendor should be leaving locked files now) * removed VC10 DLLs from WiX installers * reverted SQLite x86 DLL to 1.0.98.0 * updated WiX templates to SciexToolKit DLLs * restored SQLite DLLs in subdirectory for SciexToolKit; still not sure whether they're really needed or not, but in any case SciexToolKit does not work on x86 * added GetFileHandleTypeNumber() since file handle types differ between different versions of Windows; it's a heuristic because I haven't found a definitive list of them - reverted to previous Sciex SDK for WIFF1 files, which restores 32-bit support and avoid file locking issues (WIFF2 only works on 64-bit though) * disabled WIFF2 unit tests in 32-bit builds * fixed Linux compile * switched from GetFinalPathNameByHandleW to GetMappedFileNameW because I could not get the former to work on Windows Server 2012 (TeamCity) * fixed filename comparison case sensitivity when copying assemblies (was causing SQLite copy errors) - added check for French-style number settings which currently break the WIFF2 SciexToolKit API * changed Skyline's WIFF2 test to be conditioned on CultureInfo.NumberFormat (until 2019 when the test will start running unconditionally) * Update file list for Skyline Adminstrator Installer. * Skyline: Turn off wiff2 test when Skyline is offscreen (until 2019) to avoid the Thread and Event handle leak it causes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pwiz::util::force_close_handles_for_filepath()); a future SDK update will fix these issues