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.

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

Engine -> Framework roll blocked on dart fix test failure. #105641

Closed
zanderso opened this issue Jun 8, 2022 · 12 comments
Closed

Engine -> Framework roll blocked on dart fix test failure. #105641

zanderso opened this issue Jun 8, 2022 · 12 comments
Assignees
@pq
Labels
dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression

Comments

@zanderso
Copy link
Member

zanderso commented Jun 8, 2022

dart fix tests are blocking the Engine -> Framework roll:

https://ci.chromium.org/ui/p/flutter/builders/try/Windows%20framework_tests_misc/20038/overview from #105633 that includes the Dart roll https://dart.googlesource.com/sdk.git/+log/6a7e6decee92..68fd70178cbf, and only one other unrelated change.
@zanderso zanderso added dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression labels Jun 8, 2022
@zanderso zanderso mentioned this issue Jun 8, 2022
Merged
@zanderso
Copy link
Member Author

zanderso commented Jun 8, 2022

I see changes to dart fix in the commit log from @pq

@a-siva a-siva assigned pq Jun 8, 2022
@pq
Copy link
Contributor

pq commented Jun 8, 2022

Two of my changes could potentially be the root of this: dart-lang/sdk@3290d7a or dart-lang/sdk@467a1b0. My guess is the former but will look a bit closer.

@pq
Copy link
Contributor

pq commented Jun 8, 2022

Curiously, the flutter-engine try bot runs were clean for both changes...

https://dart-review.googlesource.com/c/sdk/+/246160 and https://dart-review.googlesource.com/c/sdk/+/246663

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jun 8, 2022
@pq
Revert "[data driven] support moving symbols across packages"
f111a2d 
This reverts commit 3290d7a.

Reason for revert: flutter engine breakage (flutter/flutter#105641)

Original change's description:
> [data driven] support moving symbols across packages
>
> See: #48997
>
> Change-Id: Iad16b9eae0523bc4bc14537af642b05efa75b6f7
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/246663
> Commit-Queue: Phil Quitslund <[email protected]>
> Reviewed-by: Brian Wilkerson <[email protected]>

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I4a83d50497d6208b7f518ca1b381ece3aab192ad
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247606
Reviewed-by: Siva Annamalai <[email protected]>
Reviewed-by: Keerti Parthasarathy <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
@zanderso
Copy link
Member Author

zanderso commented Jun 8, 2022

This can be downgraded when the roll here lands: #105650

@jason-simmons
Copy link
Member

The affected test works on my Windows setup if I revert the other dart fix patch (https://dart-review.googlesource.com/c/sdk/+/246160)

@pq
Copy link
Contributor

pq commented Jun 8, 2022

Oh! OK, that surprises me. I'll go ahead and revert that. Apologies! And thanks for the catch @jason-simmons!

@jason-simmons
Copy link
Member

jason-simmons commented Jun 8, 2022 •
edited
Loading

It works on Windows with https://dart-review.googlesource.com/c/sdk/+/246160 if I patch _getTarget to use the directory path normalization from before that PR:

  io.FileSystemEntity _getTarget(List<String> arguments) {
    var argumentCount = arguments.length;
    if (argumentCount > 1) {
      usageException('Only one file or directory is expected.');
    }
    io.Directory dir =
        argumentCount == 0 ? io.Directory.current : io.Directory(arguments[0]);
    var normalizedPath = path.canonicalize(path.normalize(dir.absolute.path));
    return io.FileSystemEntity.isDirectorySync(normalizedPath)
        ? io.Directory(normalizedPath)
        : io.File(normalizedPath);
  }

(The failing test is running dart fix --compare-to-golden in flutter\packages\flutter\test_fixes, i.e. the argumentCount == 0 case above)

@pq
Copy link
Contributor

pq commented Jun 8, 2022

Ah! Funny. I'd expect that to have been caught on our windows bots... Will have to dig in. Thanks!

@pq
Copy link
Contributor

pq commented Jun 8, 2022

Thanks a million @jason-simmons! I'm building w/ your fix locally now. (I'll take a look at adding some more tests too...)

@pq
Copy link
Contributor

pq commented Jun 8, 2022

New patch: https://dart-review.googlesource.com/c/sdk/+/247608. 🦅

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jun 9, 2022
@pq
fix path normalization for windows
8d9009d 
See: flutter/flutter#105641 (comment)

Change-Id: I176467d5885feab1c44892ea9140f619e30df223
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247608
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Srujan Gaddam <[email protected]>
@pq
Copy link
Contributor

pq commented Jun 9, 2022

Fix landed. dart-lang/sdk@8d9009d should unblock the roll.

Sorry for the noise!

@aam aam closed this as completed Jun 9, 2022
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jun 13, 2022
@pq
Reland "[data driven] support moving symbols across packages"
9a48d1b 
This reverts commit f111a2d.

Reason for revert: this change was mistakenly associated w/ a windows bot breakage (flutter/flutter#105641)

Original change's description:
> Revert "[data driven] support moving symbols across packages"
>
> This reverts commit 3290d7a.
>
> Reason for revert: flutter engine breakage (flutter/flutter#105641)
>
> Original change's description:
> > [data driven] support moving symbols across packages
> >
> > See: #48997
> >
> > Change-Id: Iad16b9eae0523bc4bc14537af642b05efa75b6f7
> > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/246663
> > Commit-Queue: Phil Quitslund <[email protected]>
> > Reviewed-by: Brian Wilkerson <[email protected]>
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Change-Id: I4a83d50497d6208b7f518ca1b381ece3aab192ad
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247606
> Reviewed-by: Siva Annamalai <[email protected]>
> Reviewed-by: Keerti Parthasarathy <[email protected]>
> Commit-Queue: Phil Quitslund <[email protected]>
> Reviewed-by: Phil Quitslund <[email protected]>
> Reviewed-by: Brian Wilkerson <[email protected]>

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I769dec0d85fc0a41048b21deec0724d8509ef8e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/248061
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Keerti Parthasarathy <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@pq pq

Labels
dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@pq @aam @zanderso @jason-simmons