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

Remove sdk or sdk_nnbd from stack traces in dart2js #41949

Closed
natebosch opened this issue May 18, 2020 · 7 comments
Closed

Remove sdk or sdk_nnbd from stack traces in dart2js #41949

natebosch opened this issue May 18, 2020 · 7 comments
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dart2js

Comments

@natebosch
Copy link
Member

Before the unfork we got org-dartlang-sdk:///sdk/lib/, after the unfork we get org-dartlang-sdk:///sdk_nnbd/lib/. Instead of hardcoding sdk_nnbd for now, and then get broken again if/when that gets removes, we should consider switching to org-dartlang-sdk:///lib/. It's a minor breaking change for package:test but I think the only impact is a degraded UX around stack traces and upgrading to the latest test runner should be a viable fix for most users.

cc @grouma @jakemac53 in case there are any concerns about old versions of package test not working with newer SDKs for this.

@natebosch natebosch added web-dart2js area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. labels May 18, 2020
@sigmundch
Copy link
Member

Just to clarify, are you seeing this in dart2js, ddc, or both sdks?

@natebosch
Copy link
Member Author

natebosch commented May 18, 2020

Only dart2js is causing our tests to fail. , and only when we build for node.

I'm not sure if we have the same behavior covered for DDC since we rely on build_runner for DDC and it's not tested in the package:test repo.

@jakemac53
Copy link
Contributor

build_runner does have some stack trace tests of its own in the _test dir I am pretty sure. We should verify that those are passing but I haven't seen a failure. I think it just looks for org-dartlang-sdk scheme.

@sigmundch
Copy link
Member

ok, we'd need to also investigate if there are downstream dependencies of this internally in our bazel integration

@sigmundch
Copy link
Member

Actually we'll likely be able to change the external build without changing the internal one, so we may be able to treat this as a separate issue

@jakemac53
Copy link
Contributor

Fwiw, here is the test code for the browser platform that breaks https://github.com/dart-lang/test/blob/master/pkgs/test/lib/src/runner/browser/platform.dart#L414. It is setting an sdk root of org-dartlang-sdk:///sdk.

So, we can't easily do anything compatible across multiple sdks other than setting it to org-dartlang-sdk:/// which will show sdk/sdk_nnbd in the path on some older sdks. I do think changing the root for the build itself to just above the lib dir is the correct choice though, assuming it makes org-dartlang-sdk paths be valid paths under the shipped sdk.

@sigmundch
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dart2js
Projects
None yet
Development

No branches or pull requests

3 participants