KEMBAR78
[build] see if we can allow long branch names by jonathanpeppers · Pull Request #9983 · dotnet/android · GitHub
Skip to content

[build] see if we can allow long branch names #9983

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Apr 2, 2025
Merged

[build] see if we can allow long branch names #9983

merged 13 commits into from
Apr 2, 2025

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Mar 27, 2025 •
edited
Loading

If you open a PR with a long branch name, it can error with:

NuGet.Build.Tasks.Pack.targets(221,5): error NU5123: Warning As Error: The file 'package/services/metadata/core-properties/d9cd2ffb8cfc41bdbfe90e7dbc7228fb.psmdcp' path, name, or both are too long. Your package might not work without long file path support. Please shorten the file path or file name.

Let's ignore NU5123 for pull requests.

I think the warning, in general, is good to know if we create a path inside a .nupkg that is too long. But for PRs, we should ignore it.

This also reverts some commits that "shorten" branch names, which should no longer be needed:

While making this work, I noticed that the CreateAllPacks target did not emit .binlog files.

I set it up to create them in the same format as we do in make:

<PropertyGroup>
  <_BinlogPrefix>-bl:$(XamarinAndroidSourcePath)bin/Build$(Configuration)/msbuild-$([System.DateTime]::Now.ToString('yyyyMMddThhmmss'))-</_BinlogPrefix>
</PropertyGroup>

Then added a suffix with each specific project name.

@jonathanpeppers
[build] see if we can allow long branch names
3f9725f 
If you open a PR with a long branch name, it can error with:

    NuGet.Build.Tasks.Pack.targets(221,5): error NU5123: Warning As Error: The file 'package/services/metadata/core-properties/d9cd2ffb8cfc41bdbfe90e7dbc7228fb.psmdcp' path, name, or both are too long. Your package might not work without long file path support. Please shorten the file path or file name.

Let's set `$(WarningAsMessages)=NU5123` for pull requests.

I think the warning, in general, is good to know if we create a path
inside a `.nupkg` that is too long. But for PRs, we should ignore it.

This also reverts some commits that "shorten" branch names, which
should no longer be needed:

* 7651b64
* 6d9b295
@jonathanpeppers
Try $(NoWarn) instead
445c00b
@jonathanpeppers
MOAR LOGGING
4c807b7
@jonathanpeppers
$(SYSTEM_PULLREQUEST_ISFORK)
4c2a1d5
@jonathanpeppers
Remove $MSBUILDLOGALLENVIRONMENTVARIABLES
576b895
@jonathanpeppers
Binlogs for all!
2d8ac82
jonathanpeppers
jonathanpeppers commented Mar 28, 2025
View reviewed changes
build-tools/create-packs/Directory.Build.props Outdated
Comment on lines 16 to 17
<!-- Allow NU5123 for pull requests, which will have $SYSTEM_PULLREQUEST_ISFORK set to true or false -->
<NoWarn Condition=" '$(SYSTEM_PULLREQUEST_ISFORK)' != '' ">$(NoWarn);NU5123</NoWarn>
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why, but this was the only $SYSTEM_PULLREQUEST_* variable set:

The rest were blank.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dovetailing with this question, what is the relevance to $(SYSTEM_PULLREQUEST_ISFORK) to ignore NU5123? Why is it (seemingly) safe to ignore NU5123 on PR builds, but not on "local"/"on my machine" builds?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking it would be good to allow NU5123 to warn (and then error) on release branches.

I don't believe any $(SYSTEM_PULLREQUEST_*) variables are set on release branches, because it isn't a pull request.

Copy link
Member

@pjcollins pjcollins Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately $(SYSTEM_PULLREQUEST_ISFORK) appears to always be set, it's present and set to False when running a CI build against a main branch.

We could try something like this instead:

 <NoWarn Condition=" '$(BUILD_DEFINITIONNAME)' == 'Xamarin.Android-PR' ">$(NoWarn);NU5123</NoWarn>

Copy link
Member Author

Choose a reason for hiding this comment

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

@pjcollins is there any other way to know it's a pull request? Or do we just need to $(NoWarn)=NU5123 no matter what?

Copy link
Member

Choose a reason for hiding this comment

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

Was late with my edit above, I think we could update this to ignore NU5123 on builds that happen on our Xamarin.Android-PR pipeline

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I see both these env vars on one of the older builds:

image

jonathanpeppers
jonathanpeppers commented Mar 28, 2025
View reviewed changes
build-tools/create-packs/Directory.Build.targets
Comment on lines +51 to +53
<PropertyGroup>
<_BinlogPrefix>-bl:$(XamarinAndroidSourcePath)bin/Build$(Configuration)/msbuild-$([System.DateTime]::Now.ToString('yyyyMMddThhmmss'))-</_BinlogPrefix>
</PropertyGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

OK this log is the same format as make:

image

@jonathanpeppers
Copy link
Member Author

InstallAndroidDependenciesTest("GoogleV2") is failing on all PRs today, so I think that is unrelated:

image

@jonathanpeppers jonathanpeppers marked this pull request as ready for review March 28, 2025 16:38
@jonpryor
Copy link
Contributor

jonpryor commented Apr 1, 2025

What are the implications of ignoring NU5123, and why is this "safe"?

NU5123 states:

A file was detected to have an installed path of longer than 200 characters. Installed path is defined as the <package_id>/<version>/target_file_path.

("200 characters" is presumably used to provide (some) "wiggle room" when NuGet packages are installed into %HOMEDRIVE%%HOMEPATH%\.nuget\packages, allowing %HOMEDRIVE%%HOMEPATH%\.nuget\packages to be 60 characters before Bad Things possibly happen.)

Is it safe to ignore NU5123 because the branch name isn't part of package_id or version?

@jonathanpeppers
Copy link
Member Author

What are the implications of ignoring NU5123, and why is this "safe"?

I want to ignore it on PRs and not builds on any other branch. PRs will be the only ones with long names.

@jonathanpeppers jonathanpeppers merged commit a078ab0 into main Apr 2, 2025
57 of 59 checks passed
@jonathanpeppers jonathanpeppers deleted the dev/peppers/battle-to-enable-really-long-branch-names-that-should-work-duh branch April 2, 2025 20:42
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pjcollins pjcollins pjcollins approved these changes

@jonpryor jonpryor Awaiting requested review from jonpryor

@dellis1972 dellis1972 Awaiting requested review from dellis1972

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jonathanpeppers @jonpryor @pjcollins