Exclude uap in WinUI package, wasdk in UWP package

[Arlodotexe]

This PR fixes an issue with our CI where the built UWP and WinUI packages included "All" multitargets.

Including both doesn't cause issues in UWP or WinUI projects normally as the correct TFM is picked up. However, in certain scenarios uap may be picked up when the winappsdk TFMs were intended, such as in the packaging project for applications using the Windows Application Packaging setup.

Closes #490 (comment)

[michael-hawker]

Ah, I see, our old setup used to do this at one point, as we had discussed whether packages should contain the other framework for Windows developers, and then hit an issue with that scenario, so we removed it at one point. That was this issue here: #220 (I added this comment to this new issue, we can classify this as a regression at least.)

[Arlodotexe]

That's right @michael-hawker. In the local solution, we allow all to include both uwp and wasdk regardless of the WinUI major version requested, to allow them to both load in our solution. It's not behavior we want from our CI due to the way other solution setups might select the wrong TFM.

[Arlodotexe]

Checking from the PR feed, it looks like this didn't change the TFMs produced 🤔

Need to investigate why. It's either a variable not being passed into the CI step, or the value is being passed and it's an issue with the underlying script.

[Arlodotexe]

Found the issue, easy mistake. Should work now.

[Arlodotexe]

@michael-hawker Looks like that did the trick, we should be good to go!

[Arlodotexe]

Curious why UWP is missing symbols though 🤔

Should we do a follow-up patch PR or try fixing it here? Seems priority to ship this patch for Windows Application Packaging project users.

[michael-hawker]

Hmm, I see the UWP ones from the main latest feed missing it too:

Though our released 8.1.240821 seems fine for both on our MainLatest feed.

We should try and understand what's happened here. Looks like it started in the first build of 8.2 (above)

Ugh, looks like our 1148 with just the version got stuck 😡 https://github.com/CommunityToolkit/Windows/actions/runs/10514400206/job/29135997093 - going to re-kick that if I can to see what that looks like for comparison as that should just be the version number change.

1153 corresponds from this PR here: #457 with our update to WASDK 1.6, so somehow the flags are getting changed because of that.

---

Our goal is to patch this into 8.1 though, so this should be unaffected and something we can do still.

But, I thought you were planning to branch this PR from main before we switched to 8.2? Why are we getting the 8.2 version numbers/changes?

Our plan should be either:

1. Fix forward and backport for the hotfix
   a. Ensure this is rebased on main and integrate as-is
   b. File a new ticket for the compiler symbols/flags, etc... to resolve for 8.2
   c. locally, create a new rel/8.1.YYMMDD branch from the rel/8.1.240821 branch and cherry-pick this specific build script change commit
   d. push that branch, validate the package in ADO feed, and push the hotfix release to NuGet
2. Backfix and re-integrate
   a. locally create a rel/8.1.YYMMDD branch and rebase this commit on top of rel/8.1.YYMMDD in that branch
   b. push that branch, validate the package in ADO feed, and push the hotfix release to NuGet
   c. merge that rel branch back into main

@azchohfi we haven't done a hotfix in our new branching model like this, so would appreciate any insights, recommendations, or feedback you have with these approaches.

[Arlodotexe]

Good catch @michael-hawker, it should still be 8.1, not 8.2.

I branched off of b99aa1f, which is tagged as our 8.1 release and is the commit just before we bumped our version to 8.2 4b6c77a on main, so that's a good question 🤔

[Arlodotexe]

Built this branch locally at the current head 952b239, no issue with version or symbols.

Must be something in the CI 🤔

[Arlodotexe]

Looks like the CI is checking out f054447 (see run), which is a merge commit of 952b239 (the commit we intended to build) into 3e6703b (latest from main).

The commit checked out by Actions does not match my local branch, which still has this branch and main well separated:

If we browse the repo at the current branch fix/nupkg/uap-in-winui-package, we can see that Directory.Build.props is still set to 8.1.

Using GitHub's compare tool fix/nupkg/uap-in-winui-package...main, we can see the 8.2 bump hasn't been merged into this branch yet.

GitHub Actions is checking out with a detached merge between this branch and main, adding commits intended for 8.2.
But why?

[Arlodotexe]

Found an open issue: actions/checkout#1359

This is one potential workaround, but we'd need variations for each job and ref source (main, pr, release, other).

- uses: 'actions/checkout@v4'
   with:
     ref: ${{ github.event.pull_request.head.sha }}

Maybe there's a better way, some other variable we can use. Will skim the docs.

[Arlodotexe]

Looks like this setup:

- name: Checkout Repository
  uses: actions/checkout@v4
  with:
    submodules: recursive
    ref: ${{ github.ref }}

is checking out via:

Fetching the repository
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +refs/pull/500/merge:refs/remotes/pull/500/merge
  From https://github.com/CommunityToolkit/Windows
   * [new ref]         refs/pull/500/merge -> pull/500/merge

using the branch ref instead of the commit hash:

Fetching the repository
  "C:\Program Files\Git\bin\git.exe" -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +f0544474d2f9bd86ea8ced7e9c2fb9f724d936b6:refs/remotes/pull/500/merge
  From https://github.com/CommunityToolkit/Windows
   * [new ref]         f0544474d2f9bd86ea8ced7e9c2fb9f724d936b6 -> pull/500/merge

Which tells us that this variable ref: ${{ github.ref }} is pre-resolved and provided as a hash by default, whereas here we've provided it as a branch name.

Either way, this resolves to an unexpected merge commit 8fc117d, a different hash but the same problem as before. It's a merge of 3e6703b and a9cb1b6.

It stands to reason that the ref refs/remotes/pull/500/merge provided by GitHub always has the merge from main already in it, though I can't access these refs from a local terminal to check.

@michael-hawker What now? Is this something we can even fix, or do we try to work around it for now by setting this branch to merge elsewhere?

[Arlodotexe]

@michael-hawker I've created a pre/rel/8.1.240911 branch from our existing rel/8.1.240821 and changed this PR base to that branch. Hopefully should resolve the merge issue we're seeing with Actions during checkout.

If all looks good here, we can:

1. Merge this PR into pre/rel/8.1.240911
2. Create a new rel/8.1.240911 branch off of pre/rel/8.1.240911 for releasing the patch.
3. Open a PR merging pre/rel/8.1.240911 or rel/8.1.240911 into main

[Arlodotexe]

Looks like some checks aren't appearing in CI now, closing and re-opening to fix.

[Arlodotexe]

@michael-hawker CI ran once through without the detached merge from main we saw before, but won't run again because of our build.yml preferences. Seems like they only kicked in when I pushed, after changing the base branch from main to pre/rel/8.1.240911. Thoughts?

Windows/.github/workflows/build.yml

Line 13 in 3e6703b

branches: [ main ]

For moving forward:

• We could ignore this for now and could keep moving, since the last two commits are cleanup and shouldn't change any behavior. The one CI run done before these two commits have validated that the issue is fixed.
• We could/should lay out some "official" process for shipping these patches without all of main, as that would include our 8.2 changes.

[Arlodotexe]

Checks still aren't running, maybe c859b6e needs to be in the target branch? 🤔

[Arlodotexe]

That didn't help... Need to figure out where GitHub is getting the policy from for this.

[michael-hawker]

Checks still aren't running, maybe c859b6e needs to be in the target branch? 🤔

I think you need it duplicated in the push trigger as well? Not 100% sure, but that's what I'd try next.

[Arlodotexe]

We merged both #504 and #505 into main, the same commits pushed to dev/rel/8.1.240912 and fix/nupkg/uap-in-winui-package (this branch).

This should have allowed CI checks to run for merging into a dev/* branch instead of just main, but that doesn't seem to have worked.

[Arlodotexe]

@michael-hawker For now, I've manually kicked off CommunityToolkit/Windows/actions/runs/10853721313. It's still not clear why checks aren't running here.

[michael-hawker]

As discussed, squashed into the prep branch. Next step is to merge into main and kick-off a hotfix for 8.1