-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Fix the "Missing ExternalProject for :" error #168403
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 the "Missing ExternalProject for :" error #168403
Conversation
bkonyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Just a couple of comments.
|
Thank you @bkonyi for your review. I applied your suggestions. |
|
@reidbaker I reformatted my code, but this seems this break the Google Testing. Could you be so kind to tell me what's going on at Borg? |
Apologies, we're experiencing infrastructure issues. I marked it manually as passing. |
|
@jmagman thank you for fixing that @reidbaker @bkonyi is there anything else I should do before this can get merged? |
|
Just @bkonyi to approve and add autosubmit. |
| if (_platform.isWindows) { | ||
| path = path | ||
| .replaceAll(r'\', r'\\') | ||
| .split(';') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we splitting on ;? This API is only mean to handle individual paths, not the contents of the PATH variable.
| expect(fsUtils.escapePath('C:/foo/bar/cool.dart'), 'C:/foo/bar/cool.dart'); | ||
| expect(fsUtils.escapePath('c:/foo/bar/cool.dart'), 'C:/foo/bar/cool.dart'); | ||
| expect(fsUtils.escapePath('x:/foo/bar/cool.dart'), 'X:/foo/bar/cool.dart'); | ||
| expect(fsUtils.escapePath('a:/foo;b:/bar'), 'A:/foo;B:/bar'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These last two use cases shouldn't be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for this missunderstanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries I also missed that. Thanks for the contributions. When you fix @bkonyi's comments we can get this landed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine now I guess. Let's wait for bkonyi and the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not a problem!
Fix the "Missing ExternalProject for :" error
This fixes #168296
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.