Skip to content

Fix API baseline check when building site extensions #27841

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
5 commits merged into from
Nov 15, 2020
Merged

Fix API baseline check when building site extensions #27841

5 commits merged into from
Nov 15, 2020

Conversation

JunTaoLuo
Copy link
Contributor

Try to fix 5.0 builds.

@ghost ghost added the area-hosting label Nov 14, 2020
@JunTaoLuo JunTaoLuo closed this Nov 14, 2020
@JunTaoLuo JunTaoLuo reopened this Nov 14, 2020
@JunTaoLuo JunTaoLuo marked this pull request as ready for review November 14, 2020 10:12
@JunTaoLuo JunTaoLuo requested a review from dougbu November 14, 2020 10:12
@JunTaoLuo
Copy link
Contributor Author

Tested in the PR build to ensure the fix is sufficient.

Not sure what the value is in including API files for the HostingStartup project. I think it would make more sense to disable the check instead. But given that the API baseline files are present, I've chosen to include the files for the build. Note that currently release/5.0 is blocked on this.

@JunTaoLuo JunTaoLuo requested a review from a team November 14, 2020 19:21
@JunTaoLuo JunTaoLuo added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode and removed area-hosting labels Nov 14, 2020
@JunTaoLuo
Copy link
Contributor Author

I'm going to merge this now to fix the build, but I'll address feedback separately if needed.

@JunTaoLuo
Copy link
Contributor Author

Turns out I can't force merge. I guess I'll wait for review.

@dougbu
Copy link
Contributor

dougbu commented Nov 14, 2020

@JunTaoLuo this will work but you're right API baselines for tools aren't useful. Since this project doesn't follow the usual conventions, I recommend removing the PublicApi.*.txt files then adding the following to HostingStartup.csproj

<!-- No need to track public APIs of this tool. -->
<AddPublicApiAnalyzers>false</AddPublicApiAnalyzers>

That matches what I did for other tools that don't have Tools in their paths.

dougbu
dougbu approved these changes Nov 14, 2020
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

If you want to save time and go with this approach, 🆗 Please follow up with a correction afterward in that case.

@JunTaoLuo
Copy link
Contributor Author

I'll disable the check

@JunTaoLuo JunTaoLuo changed the title Include API files when building site extensions Fix API baseline check when building site extensions Nov 14, 2020
@ghost
Copy link

ghost commented Nov 14, 2020

Hello @JunTaoLuo!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 807e8cc into release/5.0 Nov 15, 2020
@ghost ghost deleted the johluo/fix-build branch November 15, 2020 01:05
dougbu
dougbu reviewed Nov 15, 2020
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Thanks @JunTaoLuo❕ Please revert your ci.yml change before merging.

Let's also remember to discuss the annoyance of PR validation not building the site extensions in Teams w/ our leads on Monday or at the meeting on Thursday.

@dougbu
Copy link
Contributor

dougbu commented Nov 15, 2020

Oops I was looking at an old view of the PR. The src/SiteExtensions/Sdk/SiteExtension.targets change should be reverted in release/5.0 because it's just confusing.

@JunTaoLuo JunTaoLuo mentioned this pull request Nov 15, 2020
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@dougbu dougbu dougbu approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JunTaoLuo @dougbu