Treat CRLF like LF in LicenseEntryWithLineBreaks by motucraft · Pull Request #189336 · flutter/flutter

motucraft · 2026-07-12T04:54:05Z

LicenseEntryWithLineBreaks recognized CRLF at paragraph boundaries, but addLine() did not strip the trailing carriage return when collecting hard-wrapped lines within a paragraph. As a result, CRLF-terminated license text kept \r characters in LicenseParagraph.text and rendered differently from the equivalent LF text.

Drop a trailing \r in addLine() so a CRLF line ending is treated the same as an LF line ending. Add a test covering reflow and paragraph splitting with CRLF input.

Fixes #189313

Local validation

flutter test test/foundation/licenses_test.dart passed.
flutter analyze --flutter-repo lib/src/foundation/licenses.dart test/foundation/licenses_test.dart passed.
git diff --check passed.

I also ran the full flutter test locally. It failed due to unrelated golden pixel diffs outside this change; the new licenses_test.dart case passed during that run.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request modifies LicenseEntryWithLineBreaks to handle CRLF line endings by stripping trailing carriage returns (\r) and adds a unit test to verify this behavior. The review feedback recommends replacing the assert(lineStart < lineEnd) assertion with a safe conditional check to avoid potential debug-mode crashes on malformed license files, and expanding the test case to include a trailing carriage return to ensure robustness.

gemini-code-assist · 2026-07-12T05:13:39Z

+      assert(lineStart < lineEnd);
+      lines.add(text.substring(lineStart, lineEnd));


Using assert(lineStart < lineEnd) can cause runtime crashes in debug mode if the license text contains a line consisting of only \r (for example, if the file ends with \n\r or just \r). Since assertions are evaluated in debug mode, a malformed license file in a package could crash the developer's application when the licenses page is opened.

Instead of asserting, we should safely guard the addition to lines by checking if lineStart < lineEnd.

if (lineStart < lineEnd) { lines.add(text.substring(lineStart, lineEnd)); }

I don't think the suggested guard is correct for this state machine.
A standalone \r after \n, or an input containing only \r, is handled while the parser is in beforeParagraph, so it does not reach addLine(). For the EOF case this change is targeting, such as A\r, stripping the trailing CR still leaves lineStart < lineEnd.

Also, guarding only lines.add(...) would be incomplete because some callers expect addLine() to have produced a line before calling getParagraph().

gemini-code-assist · 2026-07-12T05:13:39Z

+    final List<LicenseParagraph> paragraphs = const LicenseEntryWithLineBreaks(
+      <String>[],
+      'A\r\nB\r\nC\r\n\r\nD\r\nE',
+    ).paragraphs.toList();


To ensure robustness against malformed inputs (such as files ending with a trailing \r or \n\r), we should include a trailing \r in the test case input. This will verify that the parser does not crash and correctly handles these edge cases.

Suggested change

final List<LicenseParagraph> paragraphs = const LicenseEntryWithLineBreaks(

<String>[],

'A\r\nB\r\nC\r\n\r\nD\r\nE',

).paragraphs.toList();

final List<LicenseParagraph> paragraphs = const LicenseEntryWithLineBreaks(

<String>[],

'A\\r\\nB\\r\\nC\\r\\n\\r\\nD\\r\\nE\\r\\n\\r',

).paragraphs.toList();

I'd prefer to keep this test focused on CRLF line endings inside normal hard-wrapped paragraphs and CRLF paragraph boundaries.

A trailing standalone \r after a line break is handled while the parser is in beforeParagraph, so it does not exercise the addLine() path fixed by this PR. A malformed EOF-only trailing \r case would be a separate robustness test, but I would rather not broaden this PR unless a maintainer asks for it.

justinmc

LGTM 👍 . I'm on board with your assessment of the Gemini review comments. I think I agree with you that we probably should have handled CRLF in addLine as well from the start. Thanks for fixing!

motucraft · 2026-07-15T00:41:41Z

Thanks for the review! Google testing is failing with "Internal smoke tests failed. Please contact a Google developer to help investigate the failure." Could you help check whether this is related to this PR?

Piinks · 2026-07-21T22:51:15Z

Can you rebase to refresh CI here? We rolled out some updates to how tests run.

motucraft · 2026-07-22T02:52:48Z

@Piinks Rebased and pushed. GitHub checks have been refreshed. Thanks!

justinmc · 2026-07-28T22:24:19Z

The failure seemed to just be because the PR was out of date, so I've updated it.

motucraft · 2026-08-03T12:25:38Z

This PR was approved on July 14 and CI has been passing since the branch update on July 28. Is there anything else needed before adding the autosubmit label?

motucraft · 2026-08-09T23:52:57Z

I'd like to raise this as a process problem, not just ask for a merge.

This PR was approved on 2026-07-14, 27 days ago. The diff has not changed since 07-22 (a49c18e), and no code changes have been requested since approval. It is now a month old and still has not landed.

The check history shows this was not simply a matter of waiting on CI. After the 07-28 branch update (c35c122, pushed by a maintainer, not by me), this PR was approved, carried the CICD label, and had every check green — the last one, Tree_analyze, completed at 07-29 05:46. It stayed in that state until 08-03 15:11, about 5 days and 9 hours.

That window did not end by landing. It ended with another master merge (b07514e, also pushed by a maintainer), and the Google testing failure that now blocks this PR first appeared on that merge, at 08-03 17:06. It has sat untriaged for a week. So the check that is red today is a consequence of that update, not the reason the PR failed to land during the 5 days it was green.

To be clear, I am not criticizing the reviews — the reviews were prompt, and the branch updates were well intentioned. What I am raising is structural:

Google testing took 1h54m, 2h50m and 7h46m across the three runs on this PR, and it is invalidated by every branch update. In practice, that makes it easy for the valid green window to expire before the required member-only action happens.
flutter-dashboard[bot] strips CICD on every branch update, and an external contributor cannot re-add it. It was re-added by hand four times here. I cannot make CI progress without a member doing that each time.
go/frob_pr/189336 is Google-internal. I cannot view, debug, or retry the one check that is blocking my own PR.
Approval and responsibility for landing are separate, and nothing assigns the latter. I asked on 08-03 what was needed, and no one has replied in the week since.

The net effect is that an approved, unmodified PR from an external contributor can be green for five days, go stale, and sit for a month, with no action available to the contributor to change that.

Concretely: could someone retry the internal tests and apply autosubmit if they come back green? And if there is a documented path for contributors in this situation, I would like to know it, because I was not able to find one.

motucraft · 2026-08-14T00:04:39Z

Friendly ping @justinmc @Piinks @AbdeMohlbi — could someone please retry the internal Google testing, and apply autosubmit if it comes back green?

Piinks · 2026-08-14T16:28:14Z

Hey @motucraft I understand your frustration here. Reviews have been delayed lately as we have been overwhelmed with agent generated PRs. We enabled PR limits soon after Github introduced the feature last month to prevent this, and have been working to catch up. I appreciate your patience while we have been working through the review queue.

A bit more context as well, when Google testing fails, that means internal tests are failing with this change. It requires we then investigate and often work with customers internally to find out what is failing and why. Sometimes it means we need to land changes to the internal code to unblock PRs. While it may seem like there is not a lot of activity on the PR, there could be a lot of work going on internally in order to unblock it.

Piinks · 2026-08-14T16:29:30Z

It ended with another master merge (b07514e, also pushed by a maintainer)

Just more context, this is common to update and re-run the internal tests under updated conditions.

Piinks · 2026-08-14T16:39:38Z

Here are our docs as well wrt code review expectations. :) https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#getting-a-code-review

motucraft · 2026-08-14T17:01:05Z

@Piinks
Thanks for the context. I understand that internal failures may require internal investigation, and that work may be happening outside the PR.

The part I’m trying to call out is that external contributors have no visibility into that state. From my side, I can only see an internal Google testing failure that I cannot inspect or retry, and no indication of whether it is being investigated, waiting for an owner, or blocked on something else.

Piinks · 2026-08-14T17:32:12Z

I am rebasing to update the base commit. I needs to be within 7 days in order to be mergable.

LicenseEntryWithLineBreaks recognized CRLF at paragraph boundaries, but addLine() did not strip the trailing carriage return when collecting hard-wrapped lines within a paragraph. As a result, CRLF-terminated license text kept \r characters in LicenseParagraph.text and rendered differently from the equivalent LF text. Drop a trailing \r in addLine() so a CRLF line ending is treated the same as an LF line ending. Add a test covering reflow and paragraph splitting with CRLF input. Fixes flutter#189313

auto-submit · 2026-08-14T18:01:17Z

autosubmit label was removed for flutter/flutter/189336, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

github-actions Bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 12, 2026

motucraft marked this pull request as ready for review July 12, 2026 05:12

gemini-code-assist Bot reviewed Jul 12, 2026

View reviewed changes

justinmc approved these changes Jul 14, 2026

View reviewed changes

AbdeMohlbi approved these changes Jul 14, 2026

View reviewed changes

AbdeMohlbi added the CICD Run CI/CD label Jul 15, 2026

flutter-dashboard Bot removed the CICD Run CI/CD label Jul 15, 2026

AbdeMohlbi added the CICD Run CI/CD label Jul 15, 2026

motucraft force-pushed the fix/license-crlf-line-endings branch from a851784 to 86571d8 Compare July 22, 2026 00:50

flutter-dashboard Bot removed the CICD Run CI/CD label Jul 22, 2026

motucraft force-pushed the fix/license-crlf-line-endings branch from 86571d8 to a49c18e Compare July 22, 2026 02:50

AbdeMohlbi added the CICD Run CI/CD label Jul 22, 2026

flutter-dashboard Bot removed the CICD Run CI/CD label Jul 28, 2026

justinmc added the CICD Run CI/CD label Jul 28, 2026

flutter-dashboard Bot removed the CICD Run CI/CD label Aug 3, 2026

AbdeMohlbi added the CICD Run CI/CD label Aug 3, 2026

Piinks force-pushed the fix/license-crlf-line-endings branch from b07514e to 1777461 Compare August 14, 2026 17:32

flutter-dashboard Bot removed the CICD Run CI/CD label Aug 14, 2026

Piinks added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Aug 14, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 14, 2026

AbdeMohlbi added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2026

auto-submit Bot added this pull request to the merge queue Aug 15, 2026

Merged via the queue into flutter:master with commit d26b43a Aug 15, 2026
35 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2026

motucraft deleted the fix/license-crlf-line-endings branch August 25, 2026 14:03

		assert(lineStart < lineEnd);
		lines.add(text.substring(lineStart, lineEnd));

Conversation

motucraft commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Local validation

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

motucraft Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

motucraft Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

motucraft commented Jul 15, 2026

Uh oh!

Piinks commented Jul 21, 2026

Uh oh!

motucraft commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Jul 28, 2026

Uh oh!

motucraft commented Aug 3, 2026

Uh oh!

motucraft commented Aug 9, 2026

Uh oh!

motucraft commented Aug 14, 2026

Uh oh!

Piinks commented Aug 14, 2026

Uh oh!

Piinks commented Aug 14, 2026

Uh oh!

Piinks commented Aug 14, 2026

Uh oh!

motucraft commented Aug 14, 2026

Uh oh!

Piinks commented Aug 14, 2026

Uh oh!

auto-submit Bot commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

motucraft commented Jul 12, 2026 •

edited

Loading

motucraft commented Jul 22, 2026 •

edited

Loading