Skip to content

Build sourcelink after command-line-api #16924

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 10, 2023
Merged

Build sourcelink after command-line-api #16924

merged 1 commit into from
Jul 10, 2023

Conversation

@NikolaMilosavljevic
Copy link
Member

@NikolaMilosavljevic NikolaMilosavljevic commented Jul 10, 2023 •
edited
Loading

Contributes to:

@NikolaMilosavljevic NikolaMilosavljevic requested a review from a team as a code owner July 10, 2023 15:55
MichaelSimons
MichaelSimons approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

These particular changes look good. In regards to dotnet/source-build#3439, what do you think the scope is around cleaning up the existing version.details.xml dependencies? I think we should at a minimum clean up the dependencies in the source-build repos - SBRP and SB-externals. Should we be removing the source-build attribute from the other repos?

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 10, 2023 •
edited
Loading

These particular changes look good. In regards to dotnet/source-build#3439, what do you think the scope is around cleaning up the existing version.details.xml dependencies? I think we should at a minimum clean up the dependencies in the source-build repos - SBRP and SB-externals. Should we be removing the source-build attribute from the other repos?

I'll update the PR description to specify that this PR contributes, but it does not completely fix dotnet/source-build#3439

My thinking is that we should remove source-build tag from all repos that are using newer SDK (except one for flow control): dotnet/sdk#31632

Should we track this as part of separate issue for source-build tag cleanup? There's likely some more opportunities for this cleanup besides sourcelink dependencies.

@MichaelSimons
Copy link
Member

MichaelSimons commented Jul 10, 2023

Should we track this as part of separate issue for source-build tag cleanup? There's likely some more opportunities for this cleanup besides sourcelink dependencies.

I think an issue specific to cleaning up the sourcelink dependencies would be best. Any other cleanup that is discovered should be tracked separately and is likely lower priority.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 10, 2023

Should we track this as part of separate issue for source-build tag cleanup? There's likely some more opportunities for this cleanup besides sourcelink dependencies.

I think an issue specific to cleaning up the sourcelink dependencies would be best. Any other cleanup that is discovered should be tracked separately and is likely lower priority.

Issue for tag removal: dotnet/source-build#3551

@NikolaMilosavljevic NikolaMilosavljevic merged commit 8062db4 into dotnet:main Jul 10, 2023
@MichaelSimons
Copy link
Member

MichaelSimons commented Jul 11, 2023 •
edited
Loading

@NikolaMilosavljevic, @mthalman - it looks like these changes may have broke the bootstrap CI legs.

⁠/vmr/src/source-build-reference-packages/artifacts/obj/ArcadeGeneratedProjects/SourceBuildIntermediate/SourceBuildIntermediate.proj : error NU1603: Warning As Error: SourceBuildIntermediate depends on Microsoft.SourceLink.AzureRepos.Git (>= 8.0.0-beta.23252.2) but Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23252.2 was not found. An approximate best match of Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23358.1 was resolved.

##[error]/vmr/src/source-build-reference-packages/artifacts/obj/ArcadeGeneratedProjects/SourceBuildIntermediate/SourceBuildIntermediate.proj(0,0): error NU1603: (NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild) Warning As Error: SourceBuildIntermediate depends on Microsoft.SourceLink.AzureRepos.Git (>= 8.0.0-beta.23252.2) but Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23252.2 was not found. An approximate best match of Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23358.1 was resolved.

Failed to restore /vmr/src/source-build-reference-packages/artifacts/obj/ArcadeGeneratedProjects/SourceBuildIntermediate/SourceBuildIntermediate.proj (in 65 ms).

I suspect this is related to the sourcelink version.details.xml dependencies not being cleaned up first, IDK for certain.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

@NikolaMilosavljevic, @mthalman - it looks like these changes may have broke the bootstrap CI legs.

⁠/vmr/src/source-build-reference-packages/artifacts/obj/ArcadeGeneratedProjects/SourceBuildIntermediate/SourceBuildIntermediate.proj : error NU1603: Warning As Error: SourceBuildIntermediate depends on Microsoft.SourceLink.AzureRepos.Git (>= 8.0.0-beta.23252.2) but Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23252.2 was not found. An approximate best match of Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23358.1 was resolved.

##[error]/vmr/src/source-build-reference-packages/artifacts/obj/ArcadeGeneratedProjects/SourceBuildIntermediate/SourceBuildIntermediate.proj(0,0): error NU1603: (NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild) Warning As Error: SourceBuildIntermediate depends on Microsoft.SourceLink.AzureRepos.Git (>= 8.0.0-beta.23252.2) but Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23252.2 was not found. An approximate best match of Microsoft.SourceLink.AzureRepos.Git 8.0.0-beta.23358.1 was resolved.

Failed to restore /vmr/src/source-build-reference-packages/artifacts/obj/ArcadeGeneratedProjects/SourceBuildIntermediate/SourceBuildIntermediate.proj (in 65 ms).

I suspect this is related to the sourcelink version.details.xml dependencies not being cleaned up first, IDK for certain.

This is failing early, building first repo, source-build-reference-packages. I'll work on creating patches for all repos to remove unwanted sourcelink dependencies.

@MichaelSimons
Copy link
Member

MichaelSimons commented Jul 11, 2023

One other thing I noticed that I question if it needs to be cleaned up it the SOURCE_LINK_BOOTSTRAP_VERSION logic in build.sh.

@MichaelSimons
Copy link
Member

MichaelSimons commented Jul 11, 2023

It looks like 8.0.0-beta.23252.2 is the version of sourcelink the SDK used in stage 1 depends on.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

One other thing I noticed that I question if it needs to be cleaned up it the SOURCE_LINK_BOOTSTRAP_VERSION logic in build.sh.

This is puzzling. This PR seems to have caused the issue, but source-build-reference-packages was always building first. Why would it just start failing now?

Old build order:

source-build-reference-packages
sourcelink
arcade
command-line-api
<other-repos>

New build order:

source-build-reference-packages
arcade
command-line-api
sourcelink
<other-repos>

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

It looks like 8.0.0-beta.23252.2 is the version of sourcelink the SDK used in stage 1 depends on.

It must be some transitive dependency coming from arcade packages as arcade now builds before sourcelink.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

Found the root cause. Arcade bootstrap package is used to build source-build-reference-packages. That package has a props file that carries versions of Arcade dependencies - tools/DefaultVersions.Generated.props, with this as one of the properties:

    <MicrosoftSourceLinkGitHubVersion>8.0.0-beta.23252.2</MicrosoftSourceLinkGitHubVersion>

Of course, PSB archive contains newer sourcelink packages. This wasn't the problem before this PR, as sourcelink was built before arcade.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

I had to implement this repo reordering as I'm enabling dotnet sourcelink tool, which consumes command-line-api content. Without reorder, command-line-api content would be pulled in from PSB archive and cause file poisons.

@MichaelSimons
Copy link
Member

MichaelSimons commented Jul 11, 2023

Found the root cause. Arcade bootstrap package is used to build source-build-reference-packages. That package has a props file that carries versions of Arcade dependencies

With the sourcelink changes in the SDK, is the Arcade reference to sourcelink still necessary?

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

Found the root cause. Arcade bootstrap package is used to build source-build-reference-packages. That package has a props file that carries versions of Arcade dependencies

With the sourcelink changes in the SDK, is the Arcade reference to sourcelink still necessary?

Not sure, but I'll try to remove it and see what happens. An alternative would be to build both sourcelink and command-line-api before arcade.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

Found the root cause. Arcade bootstrap package is used to build source-build-reference-packages. That package has a props file that carries versions of Arcade dependencies

With the sourcelink changes in the SDK, is the Arcade reference to sourcelink still necessary?

Not sure, but I'll try to remove it and see what happens. An alternative would be to build both sourcelink and command-line-api before arcade.

@mmitche with SourceLink now part of SDK, can we remove the dependency in Arcade and especially these two lines: https://github.com/dotnet/arcade/blob/2d2482f9b6ccaaac02ef39adc06b3e30b29a5c13/src/Microsoft.DotNet.Arcade.Sdk/Microsoft.DotNet.Arcade.Sdk.csproj#L70-L71

@mmitche
Copy link
Member

mmitche commented Jul 11, 2023

@NikolaMilosavljevic, @tmat was waiting for the p6 release to remove the arcade functionality. The functionality was in p5, but broken there. I believe this work can be completed now.

/cc @ViktorHofer who was also involved.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jul 11, 2023

@NikolaMilosavljevic, @tmat was waiting for the p6 release to remove the arcade functionality. The functionality was in p5, but broken there. I believe this work can be completed now.

/cc @ViktorHofer who was also involved.

@tmat, @ViktorHofer is there a draft PR or a summary of changes (if small)? I'd like to test a local VMR build to see if stage 2 build gets unblocked.

@ViktorHofer
Copy link
Member

ViktorHofer commented Jul 11, 2023

We first need dotnet/arcade#13886 to go in and then revert dotnet/arcade#13825

@NikolaMilosavljevic NikolaMilosavljevic mentioned this pull request Jul 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelSimons MichaelSimons MichaelSimons approved these changes

@mmitche mmitche Awaiting requested review from mmitche

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@NikolaMilosavljevic @MichaelSimons @mmitche @ViktorHofer