Skip to content

skyline: Fixed Visual Studio builds of Skyline.sln to use the ProteoWizard binaries matching the active configuration - #4634

Merged
nickshulman merged 3 commits into
Skyline/work/20260612_net8_portfrom
Skyline/work/20260902_net10_sln_pwiz_projects
Sep 9, 2026
Merged

skyline: Fixed Visual Studio builds of Skyline.sln to use the ProteoWizard binaries matching the active configuration#4634
nickshulman merged 3 commits into
Skyline/work/20260612_net8_portfrom
Skyline/work/20260902_net10_sln_pwiz_projects

Conversation

@nickshulman

@nickshulman nickshulman commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Stacked on #4619 (Skyline/work/20260612_net8_port). Proposed for integration into that branch.

Building Skyline.sln in Visual Studio always used the Debug ProteoWizard (pwiz-sharp) binaries, regardless of whether Debug or Release was selected in the IDE. A Release build either failed with CS0006 when no Debug pwiz-sharp build existed, or silently copied the Debug DLLs next to Skyline-daily.exe when one did.

  • Root cause: Skyline.csproj references the pwiz-sharp projects (and SkylineProcessRunner), which are not part of Skyline.sln. MSBuild's AssignProjectConfiguration finds no solution configuration for them and, because ShouldUnsetParentConfigurationAndPlatform defaults to true in IDE and solution builds, strips Configuration and Platform from the reference. The referenced project then falls back to its own defaults, Debug and AnyCPU.
  • Added an AssignOutOfSolutionProjectReferenceConfiguration target to pwiz_tools/Directory.Build.targets that runs after AssignProjectConfiguration and gives every unassigned project reference the solution's Configuration plus Platform=AnyCPU, the platform build.bat uses for pwiz-sharp
  • Passed ShouldUnsetParentConfigurationAndPlatform=false along as a global property so the fix flows down the whole out-of-solution subtree (BlibBuild -> BiblioSpec, Thermo -> Vendor.Common, etc.). Fixing only the first level built BlibBuild as Release on top of a Debug BiblioSpec, so msparserD.dll and the Debug MascotShim.dll landed in BlibBuild's Release output and Skyline's msparser.dll copy failed (MSB3030).
  • The target is conditioned on CurrentSolutionConfigurationContents, so command-line csproj builds (build.bat) carry no solution configuration and are unaffected
  • Build-system-only change; no code touched

See TODO-20260612_net8_port.md in pwiz-ai/todos

Test plan

  • Built Skyline.sln in Visual Studio in Release and confirmed the Release pwiz-sharp DLLs (e.g. Pwiz.Data.MsData.dll) are copied next to Skyline-daily.exe
  • Built Skyline.sln in Visual Studio in Debug and confirmed the Debug pwiz-sharp DLLs are used
  • Confirmed the msparser.dll copy no longer fails (MSB3030) in a Release IDE build

Co-Authored-By: Claude noreply@anthropic.com

Copilot AI lite review requested due to automatic review settings September 2, 2026 21:44
@nickshulman nickshulman added the skyline Skyline application changes - create Skyline/work branch label Sep 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The .sln now starts with an extra blank line before the required header, which can break tooling/parsers and should be corrected before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates Skyline.sln to include the pwiz-sharp (.NET 10) project tree so Visual Studio can resolve and build Skyline’s pwiz-sharp project references in the IDE (matching the net10 branch dependency graph).

Changes:

  • Added a pwiz-sharp solution folder and included 23 pwiz-sharp projects under it.
  • Added solution configuration/build mappings and nested-project mappings for the new projects.
  • (Unintended) Introduced a leading blank line before the .sln header.
File summaries
File Description
pwiz_tools/Skyline/Skyline.sln Adds pwiz-sharp projects and configuration mappings to enable VS IDE builds on the net10 branch.
Review details

Suppressed comments (1)

pwiz_tools/Skyline/Skyline.sln:90

  • This adds a second project named "Common" (there is already a "Common" project earlier in the solution). Giving the pwiz-sharp one a distinct display name avoids ambiguity in Solution Explorer and when using project search/filters.
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Common", "..\..\pwiz-sharp\pwiz\src\Common\Common.csproj", "{683B444D-65C5-4FC4-887F-1D2E31DAB743}"
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pwiz_tools/Skyline/Skyline.sln Outdated
Comment on lines 1 to 2

Microsoft Visual Studio Solution File, Format Version 12.00
Comment thread pwiz_tools/Skyline/Skyline.sln Outdated
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "pwiz-sharp", "pwiz-sharp", "{66A2598E-3509-54C4-C486-F670F742CAD7}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BiblioSpec", "..\..\pwiz-sharp\Tools\BiblioSpec\src\BiblioSpec\BiblioSpec.csproj", "{77A0E477-1970-4CF1-8385-B2E59DB14E79}"
@nickshulman

Copy link
Copy Markdown
Contributor Author

Without this change, I can't build inside Visual Studio. I get the errors in this file:
VisualStudioBuildErrors.txt

You can build from the command-line since that does something that does not actually involve "Skyline.sln", but I can't figure out how to get anything done in Visual Studio without this change.

Is this the correct fix, or am I doing something wrong?

@brendanx67

Copy link
Copy Markdown
Member

I was able to create a top-level b.bat and bs.bat/bo.bat as I currently have for master. It seems to work like master, where I first need to run the top-level .bat file, but then I can iterate without worrying about pwiz-sharp.

@brendanx67 brendanx67 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not do this. Let's continue to view ProteoWizard as a pre-build step and not part of our iterative development.

@brendanx67

Copy link
Copy Markdown
Member

I had Claude send you email from the machine where I have the traditional build Pwiz first and then iterate with a 24 csproj Skyline.sln, and I had it update new-machine-setup.md again to try to better clarify how that works now. It is much quicker than the bjam C++ build, but it is still a pre-build step with a .bat file in the root.

@chambm

chambm commented Sep 3, 2026

Copy link
Copy Markdown
Member

Let's not do this. Let's continue to view ProteoWizard as a pre-build step and not part of our iterative development.

What's the motivation for that? One of my goals for this was to avoid having to do the pre-build step. I'm open to restructuring the core library to be fewer projects. I think only the vendors need to stay separate projects.

edit: Oh, and separate projects for the C shims for Mascot and Mobilion. And maybe AgilentPatcher until we get updated DLLs from them.

@nickshulman nickshulman changed the title skyline: Added the pwiz-sharp projects to Skyline.sln to fix the Visual Studio build on the net10 branch skyline: Fixed Visual Studio builds of Skyline.sln to use the ProteoWizard binaries matching the active configuration Sep 3, 2026
@brendanx67

Copy link
Copy Markdown
Member

Let's not do this. Let's continue to view ProteoWizard as a pre-build step and not part of our iterative development.

What's the motivation for that? One of my goals for this was to avoid having to do the pre-build step. I'm open to restructuring the core library to be fewer projects. I think only the vendors need to stay separate projects.

edit: Oh, and separate projects for the C shims for Mascot and Mobilion. And maybe AgilentPatcher until we get updated DLLs from them.

Sure. Reduce it to say, 5 projects, and it would be fine. I just don't want to see the project count double to get ProteoWizard in there. And that count seems like it would also impact other solutions like Osprey and eventually even BiblioSpec, since it reads raw data for MaxQuant and other formats, right?

@chambm You are the right person to solve this. pwiz-sharp needs to keep a lower profile in a solution. Imagine someone with a single .csproj solution. What is their tolerance for adding projects to their solutions?

@chambm

chambm commented Sep 8, 2026

Copy link
Copy Markdown
Member

OK I think I (and maybe Brendan) didn't actually look at what Claude came up with here to fix in-Visual-Studio SLN builds. Now I understand the ProjectReference system already means that those external (non-solution) projects get built if necessary, but some quirk of that system when running inside Visual Studio meant that the Platform and Configuration got dropped in the process and set to default values (AnyCPU/Debug). The way the build.bat passes the Platform and Configuration as explicit parameters probably avoided that for command-line builds. But the bottom line is, with Nick's fix we get SLN build-from-clean without actually having to add the core projects to the SLN. Nice! FYI those project references are visible in the GUI here:
image

@brendanx67

Copy link
Copy Markdown
Member

OK I think I (and maybe Brendan) didn't actually look at what Claude came up with here to fix in-Visual-Studio SLN builds. Now I understand the ProjectReference system already means that those external (non-solution) projects get built if necessary, but some quirk of that system when running inside Visual Studio meant that the Platform and Configuration got dropped in the process and set to default values (AnyCPU/Debug). The way the build.bat passes the Platform and Configuration as explicit parameters probably avoided that for command-line builds. But the bottom line is, with Nick's fix we get SLN build-from-clean without actually having to add the core projects to the SLN. Nice! FYI those project references are visible in the GUI here: image

Yes, a picture in the original PR would have helped. I did not actually pull this myself and look at how it appeared in VS. So, this sounds better than I had expected. I was imagining all of the pwiz-sharp projects added at the same level as the other .csproj files, like when we added ZedGraph.

@nickshulman

Copy link
Copy Markdown
Contributor Author

To be clear, this PR has gone through two iterations in terms of how it fixes the problem that Visual Studio was always using the Debug ProteoWizard dll's even when "Release" was chosen.

The first iteration added many projects to Skyline.sln.

The current iteration uses "Directory.Build.targets" to set "ShouldUnsetParentConfigurationAndPlatform" on some things to "false" so that Visual Studio uses the currently chosen configuration to decide which dll's to use instead of always using "Debug".

@nickshulman
nickshulman merged commit a4081f7 into Skyline/work/20260612_net8_port Sep 9, 2026
12 checks passed
@nickshulman
nickshulman deleted the Skyline/work/20260902_net10_sln_pwiz_projects branch September 9, 2026 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skyline Skyline application changes - create Skyline/work branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants