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

Fix integration hot-reload test on mac. #21741

Merged
merged 1 commit into from Sep 13, 2018

Conversation

aam
Copy link
Member

@aam aam commented Sep 12, 2018

Underlying issue with breakpoint was that /var path is a symlink to a /private/var on mac. Because of that breakpoint could not be resolved.

Fixes #18441

Underlying issue with breakpoint was that /var path is a symlink to a /private/var on mac. Because of that breakpoint could not be resolved.
@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2018

Ah, thanks for digging into this!

Though I wonder - wouldn't we still expect this to work? If the user always interacts with files through the symlink (eg. nothing points to the real path), shouldn't they behave the same as if they were the real files?

If not, shouldn't the resolution be done internally - otherwise won't we need to make all breakpoints sent by editors always resolved for symlinks first?

@aam
Copy link
Member Author

aam commented Sep 12, 2018

From VM perspective breakpoint api should deal with the same files VM was loaded from. If VM was started from symlinked folders, communication should continue based on the same symlinks.
Here the issue is that the tester starts from symlinked folder, attempts to set a breakpoint via symlink too, but the engine and the vm were started from actual folder. That leads to problem when VM can't resolve symlinked file as a target for the breakpoint.

@rmacnak-google

@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2018

Here the issue is that the tester starts from symlinked folder, attempts to set a breakpoint via symlink too, but the engine and the vm were started from actual folder.

Does this mean the issue is inside flutter_tester (and therefore generally won't affect end-users)? If so, the change LGTM but I think we should open an issue about this and put a link to it in a comment against the code that it can be removed once fixed?

@aam
Copy link
Member Author

aam commented Sep 12, 2018

I believe the issue is in the way how integration test interacts with flutter run --machine daemon.dart - former requests breakpoints assuming tempDir file path it has matches exactly path flutter run runs in. That assumption is incorrect.

@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2018

I'm not sure I understand where the resolved path is coming from; isn't everything using symlinks? When we run flutter run won't the workingDirectory also by the symlinked version?

@aam
Copy link
Member Author

aam commented Sep 12, 2018

When we run flutter run won't the workingDirectory also by the symlinked version?

No, it won't.

@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2018

Oh right, I see!

Would we be better resolving the path right at the start of the test when we create it, so the symlink version is never stored anywhere, like:

tempDir = fs
    .systemTempDirectory.createTempSync('flutter_hot_reload_test_app.')
    .resolveSymbolicLinksSync();

?

@aam
Copy link
Member Author

aam commented Sep 12, 2018

I considering that, but thought that the fixing the only place where symlinks matter is better.
Besides, tempDir is a Directory, but resolveSymbolicLinksSync gives you back String.

@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2018

Ok, makes sense. We may end up with more tests using breakpoints though, so I might tweak this in future if I end up having to put this around the place.

One final question - if I remove the file:/// prefix, the test passes without this change. Do you know why the behaviour is different there (and are we concerned)?

(LGTM, though it'd be good to know the answer to the above!)

@aam
Copy link
Member Author

aam commented Sep 12, 2018

I would imagine without "file:/// prefix" the path is interpreted as a relative path, so /var/tmp/filename matches /private/var/tmp/filename.

@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2018

Ah, maybe it's something like that. I'm not sure if it makes sense for a path starting / to be treated as relative but I'm not sure it's likely to cause any issues (since the chances of overlapping paths that way seems slim).

@aam aam merged commit d61b48b into flutter:master Sep 13, 2018
@aam aam deleted the fix-integr-hot-reload-test-mac branch September 13, 2018 05:35
@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.

Hot reloads fail and breakpoints may not be hit if breakpoint scriptUris are prefixed with file://
3 participants