Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graph: Mimic quirk for sln-based builds with target name containing semicolon #9390

Closed

Conversation

dfederm
Copy link
Contributor

@dfederm dfederm commented Nov 2, 2023

Fixes #9376

There is a quirk for solution builds for targets containing semicolons. For example if you do a solution build using /t:"Clean;Build", the metaproject ends up calling the <MSBuild> task using that value, which ends up splitting it into two targes: "Clean" and "Build". However if you try the same thing on a project, it will actually end up attempting to call a target named "Clean;Build", which would also certainly fail in practice (although in theory there could be a distinct target with that crazy name).

This change just mimics that quirk for graph builds.

See linked issue for a practical repro and test cases.

@AR-May AR-May requested a review from rokonec November 14, 2023 14:34
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good.

I have a minor comment for shortening the code

Comment on lines +609 to +620
{
List<string> newEntryTargets = new(entryProjectTargets.Count);
foreach (string entryTarget in entryProjectTargets)
{
foreach (string s in ExpressionShredder.SplitSemiColonSeparatedList(entryTarget))
{
newEntryTargets.Add(s);
}
}

entryProjectTargets = newEntryTargets;
}
Copy link
Member

@JanKrivanek JanKrivanek Nov 15, 2023

Choose a reason for hiding this comment

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

Why recreating the list twice? (SplitSemiColonSeparatedList already creates new)

Suggested change
{
List<string> newEntryTargets = new(entryProjectTargets.Count);
foreach (string entryTarget in entryProjectTargets)
{
foreach (string s in ExpressionShredder.SplitSemiColonSeparatedList(entryTarget))
{
newEntryTargets.Add(s);
}
}
entryProjectTargets = newEntryTargets;
}
{
entryProjectTargets = entryProjectTargets
.SelectMany(target => ExpressionShredder.SplitSemiColonSeparatedList(target))
.ToList();
}

@yuehuang010
Copy link
Contributor

FYI, long long time ago, I added a funky feature to address solution building multiple targets, non-graph. Looks up SolutionProjectReferenceAllTargets or SlnProjectResolveProjectReference. I wasn't able to enable it because there was a high chance of a regression due to missing P2P references with a different build order. With graph build, this turns over a new leaf so perhaps consider addressing the issue directly.

@JanKrivanek
Copy link
Member

FYI, long long time ago, I added a funky feature to address solution building multiple targets, non-graph. Looks up SolutionProjectReferenceAllTargets or SlnProjectResolveProjectReference. I wasn't able to enable it because there was a high chance of a regression due to missing P2P references with a different build order. With graph build, this turns over a new leaf so perhaps consider addressing the issue directly.

@yuehuang010 - this one?: #7512

@AR-May
Copy link
Member

AR-May commented Nov 28, 2023

Team triage: It seems to be resolved via PR #9452. We decided to close this one in favor to #9452.

@AR-May AR-May closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Solution-based graph builds don't handle a quoted target list consistent with non-graph builds
6 participants