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

Regression in SourceLink Url generation when metadata is missing #7850

Closed
MattGal opened this issue Sep 8, 2021 · 8 comments
Closed

Regression in SourceLink Url generation when metadata is missing #7850

MattGal opened this issue Sep 8, 2021 · 8 comments
Assignees

Comments

@MattGal
Copy link
Member

MattGal commented Sep 8, 2021

@adiaaida FYI if you want to handle it, otherwise I think Eric's suggestion will work (setting up a thing to test that)

#7844 introduced the change, but it's evidently possible for https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/RepositoryInfo.targets#L76 a missing ScmRepositoryUrl metadata item on SourceRoot.

@MattGal MattGal self-assigned this Sep 8, 2021
@ghost ghost added the Needs Triage A new issue that needs to be associated with an epic. label Sep 8, 2021
MattGal added a commit to MattGal/arcade that referenced this issue Sep 8, 2021
@MattGal
Copy link
Member Author

MattGal commented Sep 8, 2021

I set up a quick repro, and thanks to some quick help from the Rainer on the MSBuild team was able to get this PR created:
https://github.com/dotnet/arcade/pull/7852/files

@MattGal MattGal added Ops - First Responder Epic and removed Needs Triage A new issue that needs to be associated with an epic. Epic labels Sep 8, 2021
@michellemcdaniel
Copy link
Contributor

MSBuild is so hard! Sorry about the mess up. Matt, can you also port this to release/6.0 once it goes into main?

MattGal added a commit to MattGal/arcade that referenced this issue Sep 8, 2021
Address dotnet#7850 - handle the lack of a ScmRepositoryUrl when batch updating (dotnet#7852)
@MattGal
Copy link
Member Author

MattGal commented Sep 8, 2021

MSBuild is so hard! Sorry about the mess up. Matt, can you also port this to release/6.0 once it goes into main?

Created #7854

MattGal added a commit that referenced this issue Sep 8, 2021
Address #7850 - handle the lack of a ScmRepositoryUrl when batch updating (#7852)
@ericstj
Copy link
Member

ericstj commented Sep 9, 2021

MSBuild is so hard! Sorry about the mess up

No worries, I've hit this exact same bug before.

@MattGal
Copy link
Member Author

MattGal commented Sep 9, 2021

This fix worked locally in my "artificial" setup but not in practice. back to WIP, will handle it as fast as I can.

@michellemcdaniel
Copy link
Contributor

:(

@MattGal
Copy link
Member Author

MattGal commented Sep 9, 2021

:(

Sorry I just only fixed one location, the new one fixes both and definitely works on the arcade-validation build.

@MattGal
Copy link
Member Author

MattGal commented Sep 10, 2021

I have shepherded the complete fix through on both main and release/6.0 and things look good through publishing on main. Release/6.0 doesn't seem to be set up to automatically trigger post-merge so I manually kicked it and mentioned that to @epananth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants