CI Update to Add MSBuild 17 #7469
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Build.Reason:Manual by Alma Jenks Build.Url:https://apidrop.visualstudio.com/Content%20CI/_build/results?buildId=267835&view=results source_repo.branch:release-msbuild source_repo.url:https://apidrop.visualstudio.com/_git/binaries
v-alje
requested review from
AaronRobinsonMSFT,
ajcvickers,
jkoritzinsky,
merriemcgaw,
Rick-Anderson,
RussKie and
a team
as code owners
ghost
added
the
|
Docs Build status updates of commit a4c48b8:
|
gewarren
reviewed
Dec 9, 2021
There was a problem hiding this comment.
I didn't go through the whole thing, but I'm wondering why there are so many changes unrelated to msbuild. I see you added some System.* binaries to the dependencies folder in this commit: https://apidrop.visualstudio.com/binaries/_git/binaries/commit/8e5074feabf8d7a7139946a368eb9680715c68b0?refName=refs/heads/release-msbuild. But I didn't think we were generating ECMAXML for dependencies. Thoughts?
| <param name="source">The data source query.</param> | ||
| <summary>Returns the filter repeater.</summary> | ||
| <returns>The filter.</returns> | ||
| <param name="source">To be added.</param> |
There was a problem hiding this comment.
These comments shouldn't be deleted.
| <param name="source">The data source query.</param> | ||
| <summary>Returns the filter repeater.</summary> | ||
| <returns>The filter.</returns> | ||
| <param name="source">To be added.</param> |
| @@ -40,7 +40,7 @@ | |||
| <param name="conformanceMode">One of the enumeration values that specifies the guidance on the conformance checks performed on the encoded data. | |||
There was a problem hiding this comment.
These changes should be reverted.
eerhardt
reviewed
Dec 9, 2021
| @@ -25,13 +25,13 @@ | |||
| <remarks>To be added.</remarks> | |||
| </Docs> | |||
| <Members> | |||
| <Member MemberName="Trace"> | |||
There was a problem hiding this comment.
These changes don't seem related to MSBuild 17. Can you revert them?
|
Docs Build status updates of commit b3fbd4e:
|
|
@gewarren I have reduced this to just the MSBuild changes. The unexpected changes don't seem to be coming from the dependencies. The extra API are not in any of those dll files. There are a few (System.Formats.Cbor, System.IO.Compression.Brotli, and System.Numerics.Vectors) which might be explained by the XML being in the binaries repo alongside the dlls. If the ECMA XML for those had been manually modified, they would revert. The rest I can only presume have to do with any changes made to the CI system since the last run, or differences to the generation system when Auto-Import is checked. |
gewarren
reviewed
Dec 9, 2021
| </Parameters> | ||
| <Docs> | ||
| <param name="attribute">To be added.</param> | ||
| <summary>Hook for subclasses to specify whether the given <paramref name="attribute" /> should be cloned or not.</summary> | ||
| <summary> | ||
| Hook for subclasses to specify whether the given <paramref name="attribute" /> should be cloned or not |
There was a problem hiding this comment.
Suggested change
| Hook for subclasses to specify whether the given <paramref name="attribute" /> should be cloned or not | |
| Hook for subclasses to specify whether the given <paramref name="attribute" /> should be cloned or not. |
| <Docs> | ||
| <summary> | ||
| This interface extends <see cref="T:Microsoft.Build.Framework.IBuildEngine9" /> to provide a reference to the <see cref="P:Microsoft.Build.Framework.IBuildEngine10.EngineServices" /> class. | ||
| Future engine API should be added to the class as opposed to introducing yet another version of the IBuildEngine interface. |
There was a problem hiding this comment.
Suggested change
| Future engine API should be added to the class as opposed to introducing yet another version of the IBuildEngine interface. |
| <summary>Indicates whether the current object is equal to another object of the same type.</summary> | ||
| <returns> | ||
| <see langword="true" /> if the current object is equal to the <paramref name="other" /> parameter; otherwise, <see langword="false" />.</returns> | ||
| <param name="other" /> |
There was a problem hiding this comment.
Losing some valuable comments here. Can you check if this is happening anywhere else?
There was a problem hiding this comment.
So far I've determined that MSBuild sources never had those comments, so this is one case where probably, the docs XML files had content, but MSBuild sources did not.
|
|
||
| ]]></format> | ||
| </remarks> | ||
| Note: You must use <see cref="T:Microsoft.Build.Framework.SdkResultFactory" /> to return a result. |
There was a problem hiding this comment.
This file should be reverted.
| Note: Use <see cref="T:Microsoft.Build.Framework.SdkResultFactory" /> to create instances of this class. Do not | ||
| inherit from this class. | ||
| </remarks></summary> | ||
| </summary> |
There was a problem hiding this comment.
This file should be reverted.
| @@ -92,7 +92,9 @@ | |||
| </Parameters> | |||
| <Docs> | |||
| <param name="eventSource">The events available to loggers.</param> | |||
| <summary>Subscribes to status events, which is the category for the evaluation finished event.</summary> | |||
| <summary> | |||
There was a problem hiding this comment.
This file should be reverted. The original has better formatting.
|
Also, can you address the build warnings? It's also probably a good idea to use the DocsPortingTool to backport any doc comment changes that have been made directly to this repo for MSBuild docs. |
|
@v-alje @gewarren |
|
I would guess we make the changes in both the docs repo and in MSBuild sources, so that we unblock the PR and also update the MSBuild sources. |
MSBuild: fix warnings for MSBuild 17.0
|
Would it be okay to rerun the CI without importing comments? I don't think I saw any useful comments being imported. Only useful comments being overwritten and pretty formatting being unprettified. |
The DocsPortingTool works in both directions, so I was suggesting we port comments from dotnet-api-docs to the source code. I really hate to see nice comments going to waste. |
|
@gewarren Sure. We can give it a try. I have kicked off a new CI run. It will push to the smoke-test-msbuild-no-import branch. There is one new namespace we will need to look at once it is done. |
|
Docs Build status updates of commit f1384ef:
|
|
Docs Build status updates of commit 14fc66f: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
xml/Microsoft.Build.Utilities/ToolLocationHelper.xml
xml/Microsoft.Build.Utilities/TargetDotNetFrameworkVersion.xml
For more details, please refer to the build report. If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
|
@v-alje @gewarren I did previous work to port comments back into MSBuild source, I just did not use the DocsPortingTool to do it. I'm not sure why these particular comments weren't already backported, but maybe I'll discover that in my investigation. |
|
For the one additional warning, that's fixed in #7483. Please merge that into the smoke test branch. |
MSBuild: fix xref warning
I propose for this import, we don't import any comments, that way we won't overwrite any. That way we can get the APIs updated for MSBuild 17 now, and then take the time to backport any comments to the dotnet/msbuild repo before reimporting with doc comments (if necessary). |
ghogen
reviewed
Dec 10, 2021
| @@ -83,7 +91,9 @@ | |||
| </Parameters> | |||
| <Docs> | |||
| <param name="eventSource">The events available to loggers.</param> | |||
| <summary>Initializes the logger by subscribing to events of the specified event source.</summary> | |||
| <summary> | |||
| Initializes the logger by subscribing to events of IEventSource | |||
There was a problem hiding this comment.
Suggested change
| Initializes the logger by subscribing to events of IEventSource | |
| Initializes the logger by subscribing to events of the specified event source. |
|
Docs Build status updates of commit ec703f0: ✅ Validation status: passed
This comment lists only the first 50 files in the pull request. xml/Microsoft.Build.Framework.Profiler/ProfiledLocation.xml
xml/Microsoft.Build.Framework.Profiler/ProfilerResult.xml
xml/Microsoft.Build.Utilities/TargetDotNetFrameworkVersion.xml
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
ghogen
reviewed
Dec 10, 2021
| constants when the data is being passed into an array parameter. So the workaround is to | ||
| write this in the project file: | ||
| <CreateItem AdditionalMetadata="@(OutputPathItem->'TargetPath=%(Identity)')" /></remarks> | ||
| <format type="text/markdown"><
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Build.Reason:Manual by Alma Jenks
Build.Url:https://apidrop.visualstudio.com/Content%20CI/_build/results?buildId=267835&view=results
source_repo.branch:release-msbuild
source_repo.url:https://apidrop.visualstudio.com/_git/binaries
Summary
Describe your changes here.
Fixes #Issue_Number (if available)