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

[ci] Fix remaining permanent CI warnings. #8595

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Dec 15, 2023

Fixes various CI warnings.

Specifying an exact version is not recommended on Microsoft-Hosted agents. Patch versions of Python can 
be replaced by new ones on Hosted agents without notice, which will cause builds to fail unexpectedly. It is 
recommended to specify only major or major and minor version (Example: `3` or `3.9`)

Fix: only specify major.minor as requested.

build-tools\scripts\RunTests.targets(3,3): Warning MSB4011: "C:\a\_work\1\s\Directory.Build.props" 
cannot be imported again. It was already imported at "C:\a\_work\1\s\build-tools\scripts\PrepareWindows.targets 
(3,3)". This is most likely a build authoring error. This subsequent import will be ignored. 
build-tools\scripts\RunTests.targets(7,3): Warning MSB4011: "C:\a\_work\1\s\Configuration.props" 
cannot be imported again. It was already imported at "C:\a\_work\1\s\build-tools\scripts\PrepareWindows.targets 
(11,3)". This is most likely a build authoring error. This subsequent import will be ignored.
tests\api-compatibility\api-compatibility.targets(3,3): Warning MSB4011: "C:\a\_work\1\s\Configuration.props" 
cannot be imported again. It was already imported at "C:\a\_work\1\s\build-tools\scripts\PrepareWindows.targets 
(11,3)". This is most likely a build authoring error. This subsequent import will be ignored.

Fix: add a flag denoting that we have already imported a project and a Condition to prevent duplicate imports.

build-tools\xaprepare\xaprepare\Application\Context.cs(608,42): Warning CS8604: Possible null reference 
argument for parameter 'path1' in 'string Path.Combine(string path1, string path2)'.

Fix: add null check.

EXEC(0,0): Warning : dotnet archive URL https://dotnetcli.azureedge.net/dotnet/Sdk/9.0.100-
alpha.1.23610.1/dotnet-sdk-9.0.100-alpha.1.23610.1-win-x64.zip not found

Fix: checking multiple URLs is part of the expected flow, downgrade message to "info".

EXEC(0,0): Warning : Duplicate Third Party Notice 'IronyProject/Irony' 
(old class: Xamarin.Android.Prepare.XamarinAndroidBuildTasks_IronyProject_Irony_TPN; 
new class: Xamarin.Android.Prepare.XamarinAndroidToolsAidl_IronyProject_Irony_TPN)

Fix: multiple projects may use the same dependencies, downgrade to "info".

Remaining CI warnings are temporary because our MAUI tests are failing:

image

🎉

@jpobst jpobst marked this pull request as ready for review December 15, 2023 22:36
@@ -172,7 +172,7 @@ void ProcessTPN (SortedDictionary <string, ThirdPartyNotice> licenses, ThirdPart
Log.StatusLine ($" {Context.Instance.Characters.Bullet} Processing: ", tpn.Name, ConsoleColor.Gray, ConsoleColor.White);

if (licenses.ContainsKey (tpn.Name)) {
Log.WarningLine ($"Duplicate Third Party Notice '{tpn.Name}' (old class: {licenses [tpn.Name]}; new class: {tpn})");
Log.InfoLine ($"Duplicate Third Party Notice '{tpn.Name}' (old class: {licenses [tpn.Name]}; new class: {tpn})");
Copy link
Member

Choose a reason for hiding this comment

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

@grendello does this represent some issue that we should try to fix rather than suppressing the warning?

Copy link
Contributor

@grendello grendello Jan 3, 2024

Choose a reason for hiding this comment

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

The intention was that if we see this warning, we go in and remove the duplicate, and the assumption was that it would be a very rare occurrence. So, yep, if we see it, we should remove the license duplicates (if possible) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning is caused by the TPN Irony, which is used by both Xamarin.Android.Build.Tasks and Xamarin.Android.Tools.Aidl. I considered removing one of the instances but I was afraid that might cause us to miss the TPN in the future.

For example, we remove the Xamarin.Android.Tools.Aidl version today. Then in the future we remove the need for Xamarin.Android.Build.Tasks to use Irony, so we remove that version. We would not remember that we need to add the ...Aidl version back.

It felt safer to keep both and reduce the warning than risk accidentally removing them both in the future.

But I'm open to other suggestions.

Copy link
Member

@pjcollins pjcollins left a comment

Choose a reason for hiding this comment

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

LGTM aside from the one question above.

@jonpryor jonpryor merged commit 037d286 into main Jan 12, 2024
45 of 47 checks passed
@jonpryor jonpryor deleted the fix-build-warnings branch January 12, 2024 00:28
grendello added a commit that referenced this pull request Jan 12, 2024
* main:
  [ci] Fix remaining permanent CI warnings. (#8595)
  [Xamarin.Android.Build.Tasks] remove `MonoRuntimeProvider.Bundled.20.java` (#8628)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants