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

Add missing '>' to Microsoft.WindowsAppSDK.Bootstrap.CS.targets #2358

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

kythant
Copy link
Contributor

@kythant kythant commented Apr 4, 2022

Add missing '>' to Microsoft.WindowsAppSDK.Bootstrap.CS.targets.

Added smoke test to check if props and targets are formed properly.

@ghost ghost added the needs-triage label Apr 4, 2022
@kythant kythant requested a review from DrusTheAxe April 4, 2022 19:42
@kythant
Copy link
Contributor Author

kythant commented Apr 4, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kythant kythant requested a review from DefaultRyan April 4, 2022 19:43
@jonwis
Copy link
Member

jonwis commented Apr 4, 2022

How do we get test coverage for this kind of thing in the future?

@@ -1,7 +1,7 @@
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

<PropertyGroup>
<DefineConstants
<DefineConstants>
Copy link
Contributor

@riverar riverar Apr 4, 2022

Choose a reason for hiding this comment

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

Shouldn't this entire line be deleted? There's no matching end tag... and an empty DefineConstants doesn't really make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Taking a closer look, seems like we don't even need this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DrusTheAxe Want to double check with you to see if we can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

@kythant
Copy link
Contributor Author

kythant commented Apr 4, 2022

@jonwis
I caught this error because the nightly Foundation build which is internal ran into this error in it's IntegrationTest stage which builds a WindowsAppSDK Nuget, builds the WindowsAppSDK tests from the internal repo, and run against it. However this isn't run as part of a PR validation because this stage needs access to the internal feed due the dependencies on other transport packages which are internal. External contributors wouldn't be able to see this type of validation.

To catch this during the pull request validation/checks, we would need to build a test app that makes use of this target that will directly make use of the Foundation transport package.

@riverar
Copy link
Contributor

riverar commented Apr 4, 2022

To catch this during the pull request validation/checks, we would need to build a test app that makes use of this target that will directly make use of the Foundation transport package.

Maybe in the short term, PR validation could run with something like this to verify the XML is at least well formed.

- name: smoke test targets
  shell: pwsh
  run: |
     Get-ChildItem *.targets | ForEach-Object { $_.Name; [xml](Get-Content $_) | Out-Null }

Copy link
Member

@DrusTheAxe DrusTheAxe left a comment

Choose a reason for hiding this comment

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

LGTM if you delete line 4

Rafael's verify-XML-is-WellFormed is a good (and cheap :-) suggestion. Should be applied to all XML files under /build/NuSpecs

@kythant
Copy link
Contributor Author

kythant commented Apr 4, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kythant
Copy link
Contributor Author

kythant commented Apr 4, 2022

1 similar comment
@kythant
Copy link
Contributor Author

kythant commented Apr 4, 2022

@kythant kythant merged commit 0f5e22f into main Apr 4, 2022
@kythant kythant deleted the user/kythant/FixTargets branch April 4, 2022 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants