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

Rework XAML compilation targets #17539

Merged
merged 10 commits into from
Nov 26, 2024
Merged

Conversation

MrJul
Copy link
Member

@MrJul MrJul commented Nov 17, 2024

What does the pull request do?

This PR reworks the MSBuild targets in AvaloniaBuildTasks.targets, responsible for compiling the XAML files into the final assembly.

A bit of history

Before #13840, the original intermediate assembly (in the obj folder) was effectively patched in place (with a copy of the original one kept around).
This regularly caused some file access issues in parallel builds. It also made MSBuild think that the output was always changed between builds, effectively killing incremental builds.

After #13840, the original intermediate assembly is preserved, and the XAML compilation target outputs a new assembly instead. The intermediate assembly path is updated to the new one after the compilation.
At the time of that PR, this sounded like a good idea, preventing all the issues mentioned above.

However, this triggered a cascade of issues. Anything that relied on the intermediate assembly without triggering compilation was effectively broken, finding the old assembly instead.
This included NativeAOT, ClickOnce, publishing without building... Each subsequent fix triggering new failures in seemingly unrelated areas. For example, we've had #16835 re-trigger XAML compilation for --no-build publish, effectively breaking Android release builds which rely on publish targets implicitly, causing double compilation...

How was the solution implemented?

What if instead we could have XAML compilation be part of the "standard" compilation (the CoreCompile target, responsible for running the C# compiler)?

That's possible with the TargetsTriggeredByCompilation property.
Any target referenced by this property will run as part of CoreCompile. We're free to modify the intermediate assembly here, since we didn't effectively "return" it from the target yet.

We also need to add the XAML files as inputs to that target, which is doable using the CustomAdditionalCompileInputs items.

With these changes, the target's logic is greatly simplified, and just naturally works, both for full rebuilds and incremental builds.

Build tests

It was getting quite hard to ensure that we didn't regress anything while editing these targets.

To solve that, this PR introduces a new BuildTests solution, outside of the main one.
As part of CI, BuildTests is built using the NuGet packages that have just been generated from the main build. This ensures we're mimicking a standard user's solution as much as possible.

Output assemblies of the various build tests are then checked to ensure they correctly contain compiled XAML.
This is done for:

  • Standard C# platforms (Desktop, Android, iOS, Browser)
  • publish --no-build
  • NativeAOT
  • A WPF app containing both WPF and Avalonia XAML files
  • A F# project

Fixed issues

@MrJul MrJul added bug area-infrastructure Issues related to CI/tooling infrastructur area-xaml labels Nov 17, 2024

// Standard compilation - should have compiled XAML
VerifyBuildTestAssembly("bin", "BuildTests");
VerifyBuildTestAssembly("bin", "BuildTests.Android");
Copy link
Member

Choose a reason for hiding this comment

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

Note that the original issue can't be reproduced with a plain dotnet build, with Rider it was only happened when one tries to debug the app.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0053376-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@TomEdwardsEnscape
Copy link
Contributor

That's possible with the TargetsTriggeredByCompilation property.
Any target referenced by this property will run as part of CoreCompile. We're free to modify the intermediate assembly here, since we didn't effectively "return" it from the target yet.

Fantastic! Wish I'd know about this at the start.

@maxkatz6 maxkatz6 merged commit bed48a7 into AvaloniaUI:master Nov 26, 2024
9 of 11 checks passed
@BobbyCannon
Copy link

11.2.2 does not work

11.3.999-cibuild0053376-alpha does work

@noorez
Copy link

noorez commented Nov 27, 2024

It appears that even with that Alpha, the same issue that started this: #17099 is happening again.. the obj path appears to be mangled again and the optimization step fails.

edit: the extra \Avalonia path is being added again

@MrJul
Copy link
Member Author

MrJul commented Nov 27, 2024

@noorez

Please check that your packages are all restored correctly, by verifying the effective versions in the solution explorer. This PR completely removes the extra assembly copy: the intermediate assembly path is never overwritten anymore, so you shouldn't see it at all.

We now even have a test checking this (the Android one, in release mode), which fails with a double Avalonia path before the changes, and passes after. This is also the case with your repository https://github.com/noorez/AvaloniaTest17099 which builds fine with version 11.3.999-cibuild0053376-alpha.

Please provide another MSBuild binary log is you encounter any further failure.

@noorez
Copy link

noorez commented Nov 27, 2024

interestingly, even though I've explicitly used that nightly build version it appears that '11.3.999-cibuild0053380-alpha' got used instead. does this nightly contain that fix as well?

@MrJul
Copy link
Member Author

MrJul commented Nov 27, 2024

interestingly, even though I've explicitly used that nightly build version it appears that '11.3.999-cibuild0053380-alpha' got used instead. does this nightly contain that fix as well?

No, 53380 is a nightly build (from main branch) from 9 days ago. For PRs, you need the nuget-feed-all feed set up. That said, this PR is now merged so you should be able to use the very latest nightly build 53568.

@noorez
Copy link

noorez commented Nov 27, 2024

ahh. I see what happened. I had that feed disabled by mistake :(. it builds as expected.

apologies! Thanks!

@MrJul MrJul deleted the fix/xaml-compilation branch December 21, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Issues related to CI/tooling infrastructur area-xaml bug
Projects
None yet
7 participants