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

Make observatory sources strong-mode clean #32503

Closed
mkustermann opened this issue Mar 12, 2018 · 16 comments

Comments

Projects
None yet
7 participants
@mkustermann
Copy link
Member

commented Mar 12, 2018

We need to make observatory strong-mode clean, at the latest when dart2js switches to Dart 2.0.

/cc @rmacnak-google @a-siva

@sigmundch

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

I've just raised the priority: dart2js now defaults to Dart 2.0 for internal and external users. We are about to flip the entire test coverage in the bots to mostly run in Dart 2.0 as well. Once observatory is migrated, we should be able to remove support for Dart1 in dart2js.

@rmacnak-google

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

This is dependent on a Dart 2-capable package:charted. Assigning to Dart4Web.

@rmacnak-google rmacnak-google removed the area-vm label Jul 2, 2018

dart-bot pushed a commit that referenced this issue Jul 2, 2018

Add strong section for observatory_ui.status
We are about to add dart2js coverage in Dart 2.0 for observatory_ui in the bots,
this adds a section to reflect the current state of the observatory_ui tests.

Bug: #32503
Change-Id: I5fa01a4b9e09dac371722bbff977c69385f09adb
Reviewed-on: https://dart-review.googlesource.com/63447
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Sigmund Cherem <sigmund@google.com>
@sigmundch

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Mmm - it appears that this package is or almost is Dart-2 capable, but that the latest changes are not published yet. I reached out to the maintainers to get some guidance on how to proceed. @rmacnak-google, I cc'd you on the message as well.

@dgrove

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

Assigning back to @rmacnak-google - please work with whoever is needed to ensure that observatory and its transitive dependencies are Dart 2-compatible.

@dgrove dgrove assigned rmacnak-google and a-siva and unassigned dgrove Jul 3, 2018

@mit-mit

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

@rmacnak-google are you looking at this one?

@natebosch

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

I don't understand how https://github.com/dart-lang/observatory_pub_packages works, or why we have yet another approach for importing third_party packages...

Can someone familiar with it please pull in the latest charted from git? I think the problem is likely that it isn't published with Dart 2 compatible code, but is fixed in the repo.

I also filed google/charted#237 to request a publish.

@natebosch

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

apparently there are some required fixes that aren't in the git repo either. This will take some work to get exported.

If possible, could we limit the flag usage to within the SDK and keep support internally for Dart 1 mode in Dart2Js? If we consider it blocking for Dart2Stable I can take a look, but if we can punt on it I think there's likely work that is more impactful for our users.

@sigmundch

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

Just created google/charted#238 to upgrade the charted git repo with more fixes.

@rmacnak-google could you please test if this is enough to unblock you by using a git-dependency on my forked repo?

For example

dependencies:
  charted:
    git:
      url: git://github.com/sigmundch/charted
      ref: updates_for_dart2
@mit-mit

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

@sigmundch

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

The pull request landed, but has not been uploaded to pub. You can use it if you set the pubspec to:

dependencies:
  charted:
    git:
      url: git://github.com/google/charted
      ref: ba00ac4a41f02b7df9d490eae6307fa059fac5b6
@rmacnak-google

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

Thanks Siggi! I'm able to make progress locally patching that in.

dart-bot pushed a commit that referenced this issue Jul 11, 2018

[observatory] Progress toward static mode compatibility.
Bug: #32503
Change-Id: I0251eb730e7781491e7d7b50d2c6c5cdf30e6352
Reviewed-on: https://dart-review.googlesource.com/64460
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Zach Anderson <zra@google.com>
@dgrove

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@rmacnak-google is this issue ready be closed now that https://dart-review.googlesource.com/c/sdk/+/64460 has landed, or is there more to do?

dart-bot pushed a commit that referenced this issue Jul 13, 2018

[observatory] Progress toward static mode compatibility.
Bug: #32503
Change-Id: I4c3358dcc69aa26e3d296275f6ec4da22814c162
Reviewed-on: https://dart-review.googlesource.com/64820
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

dart-bot pushed a commit that referenced this issue Jul 16, 2018

[observatory] Progress toward static mode compatibility.
Bug: #32503
Change-Id: Ic4390dce8874aab4aaa87fa4153aef668f28738e
Reviewed-on: https://dart-review.googlesource.com/64982
Reviewed-by: Zach Anderson <zra@google.com>

dart-bot pushed a commit that referenced this issue Jul 17, 2018

[observatory] Progress toward static mode compatibility.
(Mostly package:unittest -> package:test)

Bug: #32503
Change-Id: I3486cceb97792707b2dc92c1ee78372731bfb92f
Reviewed-on: https://dart-review.googlesource.com/65202
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

dart-bot pushed a commit that referenced this issue Jul 17, 2018

[observatory] Switch to static mode.
Bug: #32503
Change-Id: I9ff65d1598031473fa8ca97a27e340e722f038fa
Reviewed-on: https://dart-review.googlesource.com/65382
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
@rmacnak-google

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Dart 2 regresses code size by 25%.

compressed_observatory_archive.tar: 823164 -> 1032166
(fbd9c4e -> 296319d)

@sigmundch

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

@rmacnak-google - what flags where used to compile observatory before and after?

We may want to use --omit-implicit-checks here (which is similar to what we used to have with --trust-type-annotations)

@sigmundch

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

(and woohoo! we can now delete Dart1 support from dart2js!)

@sigmundch

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Just found the GN rule, I see that no flags were being used before either (other than --minify). Please give the --omit-implicit-checks a shot, we are including a lot of reified types by default and we don't have the level of optimizations we want yet without the flag.

dart-bot pushed a commit that referenced this issue Jul 20, 2018

[observatory] Progress toward static mode compatibility.
(Mostly package:unittest -> package:test)

Bug: #32503
Change-Id: I3486cceb97792707b2dc92c1ee78372731bfb92f
Reviewed-on: https://dart-review.googlesource.com/65202
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

dart-bot pushed a commit that referenced this issue Jul 20, 2018

[observatory] Switch to static mode.
Bug: #32503
Change-Id: I9ff65d1598031473fa8ca97a27e340e722f038fa
Reviewed-on: https://dart-review.googlesource.com/65382
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.