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

Switch to URIs for breakpoints and unskip tests on Windows #22510

Merged
merged 4 commits into from Oct 19, 2018
Merged

Switch to URIs for breakpoints and unskip tests on Windows #22510

merged 4 commits into from Oct 19, 2018

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Oct 1, 2018

addBreakpointWithScriptUri expects Uris. By coincidence, FS paths work on Mac/Linux but they fail on Windows. One of the issues in the skip comment is fixed, the other one seems not relevant here.

@DanTup
Copy link
Contributor Author

DanTup commented Oct 1, 2018

I think this needs #22507 in order to pass (I had a cached build of tools when I ran this). I'll rebase and try again once that's done.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM, please make sure bots are happy before merging.

@DanTup
Copy link
Contributor Author

DanTup commented Oct 3, 2018

The failure looks like a repeat of #21741 caused by symlinks in the temp paths coming from macOS (we don't handle breakpoints sent for symlinked paths, since the VM is using the real underlying path).

I've applied that fix (which only applied to one test file previously) to all integration tests to avoid this; hopefully it'll make everything green. (@goderbauer please check the new changeset and confirm if you're still happy with this).

@DanTup
Copy link
Contributor Author

DanTup commented Oct 19, 2018

@goderbauer I pushed extra changes to handle symlinks since you approved this, let me know if you're still happy for me to land this.

(I'll rebase too since it's been a while)

addBreakpointWithScriptUri expects Uris. By coincidence, FS paths work on Mac/Linux but they fail on Windows. One of the issues in the skip comment is fixed, the other one seems not relevant here.
The default temp folders we get include symlinks which breaks breakpoints.
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -11,6 +11,14 @@ import 'package:flutter_tools/src/base/process_manager.dart';

import '../src/common.dart';

/// Creates a etmporary directory but resolves any symlinks to return the real
Copy link
Member

Choose a reason for hiding this comment

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

nit: spelling "etmporary"

@DanTup DanTup merged commit f87a2a3 into flutter:master Oct 19, 2018
@DanTup DanTup deleted the use-uris-for-breakpoints branch October 19, 2018 11:51
@DanTup
Copy link
Contributor Author

DanTup commented Oct 19, 2018

Ug, I didn't spot flutter-build was missing from this, so I landed it while the build was red 😕

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
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.

None yet

3 participants