-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update to compiler v20160713 #421
Conversation
@@ -121,7 +121,8 @@ def get(*args): | |||
|
|||
# Ignore missing goog.require since we assume the whole library is | |||
# already included. | |||
opts = ['--jscomp_off=missingRequire', '--checks-only', '-O', 'SIMPLE'] | |||
opts = ['--jscomp_off=missingRequire', '--jscomp_off=lintChecks', |
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.
Are we no longer passing lintChecks? Why is that?
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.
Regrettably, the compiler contains two different variants of the "missing require" check (MISSING_REQUIRE_WARNING, MISSING_REQUIRE_CALL_WARNING). Some legacy code is preventing MISSING_REQUIRE_CALL_WARNING from being included with the "missingRequire" diagnostic group but rather it is included in "lintChecks".
Said differently, all of the lintChecks besides MISSING_REQUIRE_CALL_WARNING still pass. Without adding lintChecks we get the following errors:
/home/ahochhaus/shaka-player2/demo/asset_section.js:30: ERROR - missing require: 'shakaAssets.testAssets'
shakaAssets.testAssets.forEach(function(asset) {
^
/home/ahochhaus/shaka-player2/demo/asset_section.js:194: ERROR - missing require: 'shaka.log'
shaka.log.debug('load() interrupted');
^
/home/ahochhaus/shaka-player2/demo/configuration_section.js:59: ERROR - missing require: 'shakaDemo.player_'
shakaDemo.player_.configure(/** @type {shakaExtern.PlayerConfiguration} */({
^
/home/ahochhaus/shaka-player2/demo/configuration_section.js:71: ERROR - missing require: 'shakaDemo.controls_'
shakaDemo.controls_.showTrickPlay(event.target.checked);
^
/home/ahochhaus/shaka-player2/demo/info_section.js:28: ERROR - missing require: 'shakaDemo.player_'
shakaDemo.player_.addEventListener(
^
/home/ahochhaus/shaka-player2/demo/info_section.js:107: ERROR - missing require: 'shakaDemo.player_'
var tracks = shakaDemo.player_.getTracks();
^
/home/ahochhaus/shaka-player2/demo/log_section.js:91: ERROR - missing require: 'shaka.log'
shaka.log.setLevel(shaka.log.currentLevel);
^
/home/ahochhaus/shaka-player2/demo/main.js:79: ERROR - missing require: 'shaka.log'
shaka.log.setLevel(shaka.log.Level.DEBUG);
^
/home/ahochhaus/shaka-player2/demo/main.js:84: ERROR - missing require: 'shaka.polyfill'
shaka.polyfill.installAll();
^
/home/ahochhaus/shaka-player2/demo/main.js:86: ERROR - missing require: 'shaka.Player'
if (!shaka.Player.isBrowserSupported()) {
^
/home/ahochhaus/shaka-player2/demo/main.js:112: ERROR - missing require: 'shaka.Player'
shaka.Player.probeSupport().then(function(support) {
^
/home/ahochhaus/shaka-player2/test/cancelable_chain_unit.js:185: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.delay(0.1).then(function() {
^
/home/ahochhaus/shaka-player2/test/dash_parser_content_protection_unit.js:33: ERROR - missing require: 'shaka.net.NetworkingEngine'
var retry = shaka.net.NetworkingEngine.defaultRetryParameters();
^
/home/ahochhaus/shaka-player2/test/dash_parser_content_protection_unit.js:125: ERROR - missing require: 'shaka.util.Uint8ArrayUtils'
initData: shaka.util.Uint8ArrayUtils.fromBase64(base64)
^
/home/ahochhaus/shaka-player2/test/dash_parser_content_protection_unit.js:219: ERROR - missing require: 'shaka.test.Dash'
var expected = shaka.test.Dash.makeManifestFromStreamSets([
^
/home/ahochhaus/shaka-player2/test/dash_parser_live_unit.js:30: ERROR - missing require: 'shaka.net.NetworkingEngine'
var retry = shaka.net.NetworkingEngine.defaultRetryParameters();
^
/home/ahochhaus/shaka-player2/test/dash_parser_live_unit.js:430: ERROR - missing require: 'shaka.util.StringUtils'
var data = shaka.util.StringUtils.toUTF8(manifest);
^
/home/ahochhaus/shaka-player2/test/dash_parser_live_unit.js:853: ERROR - missing require: 'shaka.test.Dash'
shaka.test.Dash.makeReference('s4.mp4', 4, 30, 40)
^
/home/ahochhaus/shaka-player2/test/dash_parser_manifest_unit.js:27: ERROR - missing require: 'shaka.test.Dash'
parser = shaka.test.Dash.makeDashParser();
^
/home/ahochhaus/shaka-player2/test/dash_parser_manifest_unit.js:555: ERROR - missing require: 'shaka.util.StringUtils'
var data = shaka.util.StringUtils.toUTF8(source);
^
/home/ahochhaus/shaka-player2/test/dash_parser_segment_base_unit.js:30: ERROR - missing require: 'shaka.test.Dash'
parser = shaka.test.Dash.makeDashParser();
^
/home/ahochhaus/shaka-player2/test/dash_parser_segment_list_unit.js:30: ERROR - missing require: 'shaka.test.Dash'
parser = shaka.test.Dash.makeDashParser();
^
/home/ahochhaus/shaka-player2/test/dash_parser_segment_list_unit.js:33: ERROR - missing require: 'shaka.test.Dash'
shaka.test.Dash.makeTimelineTests('SegmentList', '', [
^
/home/ahochhaus/shaka-player2/test/dash_parser_segment_template_unit.js:30: ERROR - missing require: 'shaka.test.Dash'
parser = shaka.test.Dash.makeDashParser();
^
/home/ahochhaus/shaka-player2/test/dash_parser_segment_template_unit.js:33: ERROR - missing require: 'shaka.test.Dash'
shaka.test.Dash.makeTimelineTests(
^
/home/ahochhaus/shaka-player2/test/data_uri_plugin_unit.js:22: ERROR - missing require: 'shaka.net.NetworkingEngine'
retryParameters = shaka.net.NetworkingEngine.defaultRetryParameters();
^
/home/ahochhaus/shaka-player2/test/data_uri_plugin_unit.js:78: ERROR - missing require: 'shaka.util.StringUtils'
var data = shaka.util.StringUtils.fromBytesAutoDetect(response.data);
^
/home/ahochhaus/shaka-player2/test/data_uri_plugin_unit.js:87: ERROR - missing require: 'shaka.net.NetworkingEngine'
shaka.net.NetworkingEngine.makeRequest([uri], retryParameters);
^
/home/ahochhaus/shaka-player2/test/data_uri_plugin_unit.js:88: ERROR - missing require: 'shaka.net.DataUriPlugin'
shaka.net.DataUriPlugin(uri, request)
^
/home/ahochhaus/shaka-player2/test/db_engine_unit.js:31: ERROR - missing require: 'shaka.offline.DBEngine'
shaka.offline.DBEngine.deleteDatabase().then(function() {
^
/home/ahochhaus/shaka-player2/test/db_engine_unit.js:117: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.expectToEqualError(error, expectedError);
^
/home/ahochhaus/shaka-player2/test/db_engine_unit.js:126: ERROR - missing require: 'shaka.test.Util'
return shaka.test.Util.delay(0.001);
^
/home/ahochhaus/shaka-player2/test/drm_engine_integration.js:47: ERROR - missing require: 'shaka.media.DrmEngine'
var supportTest = shaka.media.DrmEngine.probeSupport()
^
/home/ahochhaus/shaka-player2/test/drm_engine_integration.js:73: ERROR - missing require: 'shaka.net.HttpPlugin'
shaka.net.HttpPlugin(audioSegmentUri, dummyRequest)
^
/home/ahochhaus/shaka-player2/test/drm_engine_integration.js:282: ERROR - missing require: 'shaka.net.NetworkingEngine'
retryParameters: shaka.net.NetworkingEngine.defaultRetryParameters(),
^
/home/ahochhaus/shaka-player2/test/drm_engine_integration.js:350: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.expectToEqualError(
^
/home/ahochhaus/shaka-player2/test/drm_engine_integration.js:386: ERROR - missing require: 'shaka.test.Util'
return Promise.race([shaka.test.Util.delay(6), onErrorCalled]);
^
/home/ahochhaus/shaka-player2/test/drm_engine_unit.js:99: ERROR - missing require: 'shaka.net.NetworkingEngine'
var retryParameters = shaka.net.NetworkingEngine.defaultRetryParameters();
^
/home/ahochhaus/shaka-player2/test/drm_engine_unit.js:563: ERROR - missing require: 'shaka.util.StringUtils'
var initData = JSON.parse(shaka.util.StringUtils.fromUTF8(
^
/home/ahochhaus/shaka-player2/test/drm_engine_unit.js:898: ERROR - missing require: 'shaka.net.DataUriPlugin'
return shaka.net.DataUriPlugin(request.uris[0], request);
^
/home/ahochhaus/shaka-player2/test/drm_engine_unit.js:910: ERROR - missing require: 'shaka.util.StringUtils'
shaka.util.StringUtils.fromBytesAutoDetect(licenseBuffer);
^
/home/ahochhaus/shaka-player2/test/drm_engine_unit.js:942: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.expectToEqualError(error, new shaka.util.Error(
^
/home/ahochhaus/shaka-player2/test/drm_engine_unit.js:1271: ERROR - missing require: 'shaka.test.Util'
return shaka.test.Util.delay(0.1);
^
/home/ahochhaus/shaka-player2/test/ebml_parser_unit.js:238: ERROR - missing require: 'shaka.util.EbmlParser'
shaka.util.EbmlParser.getVintValue_(data);
^
/home/ahochhaus/shaka-player2/test/fake_event_target_unit.js:132: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.delay(0.1).then(function() {
^
/home/ahochhaus/shaka-player2/test/http_plugin_unit.js:48: ERROR - missing require: 'shaka.net.NetworkingEngine'
retryParameters = shaka.net.NetworkingEngine.defaultRetryParameters();
^
/home/ahochhaus/shaka-player2/test/http_plugin_unit.js:128: ERROR - missing require: 'shaka.net.NetworkingEngine'
var request = shaka.net.NetworkingEngine.makeRequest(
^
/home/ahochhaus/shaka-player2/test/http_plugin_unit.js:130: ERROR - missing require: 'shaka.net.HttpPlugin'
shaka.net.HttpPlugin(uri, request)
^
/home/ahochhaus/shaka-player2/test/networking_engine_unit.js:49: ERROR - missing require: 'shaka.net.NetworkingEngine'
shaka.net.NetworkingEngine.registerScheme('reject', rejectScheme);
^
/home/ahochhaus/shaka-player2/test/networking_engine_unit.js:239: ERROR - missing require: 'shaka.net.NetworkingEngine'
shaka.net.NetworkingEngine.unregisterScheme('resolve');
^
/home/ahochhaus/shaka-player2/test/networking_engine_unit.js:630: ERROR - missing require: 'shaka.net.NetworkingEngine'
shaka.net.NetworkingEngine.defaultRetryParameters();
^
/home/ahochhaus/shaka-player2/test/networking_engine_unit.js:631: ERROR - missing require: 'shaka.net.NetworkingEngine'
return shaka.net.NetworkingEngine.makeRequest([uri], retryParameters);
^
/home/ahochhaus/shaka-player2/test/offline_integration.js:33: ERROR - missing require: 'shaka.Player'
var supportPromise = shaka.Player.probeSupport()
^
/home/ahochhaus/shaka-player2/test/offline_integration.js:41: ERROR - missing require: 'shaka.offline.DBEngine'
Promise.all([shaka.offline.DBEngine.deleteDatabase(), supportPromise])
^
/home/ahochhaus/shaka-player2/test/offline_integration.js:115: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.expectToEqualError(e, expected);
^
/home/ahochhaus/shaka-player2/test/offline_integration.js:129: ERROR - missing require: 'shaka.test.Util'
return shaka.test.Util.delay(5);
^
/home/ahochhaus/shaka-player2/test/player_unit.js:120: ERROR - missing require: 'shaka.log'
shaka.log.debug('finished unload 2');
^
/home/ahochhaus/shaka-player2/test/player_unit.js:253: ERROR - missing require: 'shaka.media.ManifestParser'
shaka.media.ManifestParser.registerParserByMime('undefined', factory1);
^
/home/ahochhaus/shaka-player2/test/player_unit.js:335: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.delay(0.5).then(function() {
^
/home/ahochhaus/shaka-player2/test/player_unit.js:1223: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.expectToEqualError(
^
/home/ahochhaus/shaka-player2/test/simple_abr_manager_unit.js:278: ERROR - missing require: 'shaka.test.Util'
var delay = shaka.test.Util.fakeEventLoop(
^
/home/ahochhaus/shaka-player2/test/storage_unit.js:636: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.expectToEqualError(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_integration.js:147: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
timeline = shaka.test.StreamingEngineUtil.createFakePresentationTimeline(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_integration.js:205: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
shaka.test.StreamingEngineUtil.boundsCheckPosition.bind(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_integration.js:209: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
shaka.test.StreamingEngineUtil.getNumSegments.bind(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_integration.js:214: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
netEngine = shaka.test.StreamingEngineUtil.createFakeNetworkingEngine(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_integration.js:257: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
manifest = shaka.test.StreamingEngineUtil.createManifest(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_integration.js:293: ERROR - missing require: 'shaka.net.NetworkingEngine'
retryParameters: shaka.net.NetworkingEngine.defaultRetryParameters(),
^
/home/ahochhaus/shaka-player2/test/streaming_engine_unit.js:232: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
timeline = shaka.test.StreamingEngineUtil.createFakePresentationTimeline(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_unit.js:248: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
netEngine = shaka.test.StreamingEngineUtil.createFakeNetworkingEngine(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_unit.js:266: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
manifest = shaka.test.StreamingEngineUtil.createManifest(
^
/home/ahochhaus/shaka-player2/test/streaming_engine_unit.js:304: ERROR - missing require: 'shaka.test.StreamingEngineUtil'
shaka.test.StreamingEngineUtil.createMockVideoStream(6);
^
/home/ahochhaus/shaka-player2/test/streaming_engine_unit.js:1730: ERROR - missing require: 'shaka.net.NetworkingEngine'
retryParameters: shaka.net.NetworkingEngine.defaultRetryParameters(),
^
/home/ahochhaus/shaka-player2/test/support_check.js:20: ERROR - missing require: 'shaka.Player'
if (!shaka.Player.isBrowserSupported()) {
^
/home/ahochhaus/shaka-player2/test/support_check.js:22: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.cancelAllRemainingSpecs();
^
/home/ahochhaus/shaka-player2/test/util/dash_parser_util.js:26: ERROR - missing require: 'shaka.net.NetworkingEngine'
var retry = shaka.net.NetworkingEngine.defaultRetryParameters();
^
/home/ahochhaus/shaka-player2/test/util/dash_parser_util.js:102: ERROR - missing require: 'shaka.util.StringUtils'
var manifestData = shaka.util.StringUtils.toUTF8(manifestText);
^
/home/ahochhaus/shaka-player2/test/util/dash_parser_util.js:110: ERROR - missing require: 'shaka.test.Util'
shaka.test.Util.expectToEqualError(error, expectedError);
^
/home/ahochhaus/shaka-player2/test/util/manifest_generator.js:262: ERROR - missing require: 'shaka.util.Uint8ArrayUtils'
var buffer = shaka.util.Uint8ArrayUtils.fromBase64(base64);
^
/home/ahochhaus/shaka-player2/test/util/manifest_generator.js:541: ERROR - missing require: 'goog.asserts'
goog.asserts.assert(streamSet.streams.length > 0,
^
/home/ahochhaus/shaka-player2/test/util/memory_db_engine.js:85: ERROR - missing require: 'shaka.util.MapUtils'
shaka.util.MapUtils.values(this.getStore_(storeName)).forEach(callback);
^
/home/ahochhaus/shaka-player2/test/util/memory_db_engine.js:135: ERROR - missing require: 'goog.asserts'
goog.asserts.assert(storeName in this.stores_,
^
/home/ahochhaus/shaka-player2/test/util/stream_generator.js:482: ERROR - missing require: 'goog.asserts'
goog.asserts.assert(
^
/home/ahochhaus/shaka-player2/test/util/stream_generator.js:500: ERROR - missing require: 'shaka.log'
shaka.log.debug('tfdt box is version 1.');
^
/home/ahochhaus/shaka-player2/test/util/util.js:277: ERROR - missing require: 'shaka.log'
shaka.log.setLevel(shaka.log.Level.INFO);
^
/home/ahochhaus/shaka-player2/test/util/util.js:279: ERROR - missing require: 'shaka.polyfill'
shaka.polyfill.installAll();
^
/home/ahochhaus/shaka-player2/test/vtt_text_parser_unit.js:280: ERROR - missing require: 'shaka.util.StringUtils'
var data = shaka.util.StringUtils.toUTF8(string);
^
/home/ahochhaus/shaka-player2/test/vtt_text_parser_unit.js:282: ERROR - missing require: 'shaka.media.VttTextParser'
shaka.media.VttTextParser(data);
^
88 error(s), 0 warning(s)
Do you know of a better solution?
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.
Not off-hand, but I do note that all of those lint check failures are about goog.requires in demo/ and test/, which do not use goog.require in the first place. I would strongly prefer not to disable useful checks on the library just to exclude them from test and demo app code.
Thanks for the very prompt review! PTAL |
@joeyparrish Could you please take another look at this updated change? |
@@ -237,6 +237,11 @@ shaka.offline.DownloadManager.prototype.updateProgress_ = function() { | |||
(this.givenBytesTotal_ + this.bandwidthBytesTotal_); | |||
|
|||
goog.asserts.assert(this.manifest_, 'Must not be destroyed'); | |||
/** | |||
* TODO(hochhaus): Resolve circular dependency. |
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.
Filed as #431 and assigned to one of my teammates.
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.
We pushed a fix for #431, so you should be able to rebase your work and see this fixed.
I recommend splitting this PR up. Most of these changes are easy to accept (fixing semicolons, annotations, etc.), so we can go ahead and land those. I don't think we are ready to land the new compiler, though, until we find a way to address the lint check errors, and that may take more time. If we split up the PR, we can land the less controversial parts relatively quickly and avoid having to rebase those changes many times as we continue to push changes from our end. |
@joeyparrish Thanks for your feedback. I broke up this change into pull reuests per your recommendation hoping to make them a bit easier to merge. This change depends upon #453, #454, #455, #456, #457, #458, #459. However all of the other changes should be independent of one another and can be reviewed/merged in any order. PTAL |
Updated to include the {width,height} string to number conversion. PTAL |
* Drop unnecessary type alternation in SegmentIndex unit tests * Drop unnecessary bind in StreamingEngine unit tests * Drop unnecessary expose annotations in Pssh * Add comments about quoted access in cast unit tests * Move afterAll() position in cast unit tests * Make return type for PublicPromise constructor more specific * Define a type for Util.eventLoop return value Related to PR #421 Change-Id: I092a8ff366b4ac4ea868dd3f4fbe4e3d63a2167f
Testing in progress... |
Failure:
|
Perhaps this needs a quick rebase? |
Rebased. PTAL |
Testing in progress... |
Failure:
|
Strange, this did not happen locally for me. Looking more. |
Hmmm, I'm having no luck reproducing this. Do you have any hints for what might be different on the shaka-player-bot machine? |
I'm getting two failures on my Linux workstation, neither of which make any sense to me:
I believe these are both compiler bugs. The first is a failure to infer types in an override, even though they are identical. The second is clearly not correct. |
Replacing Still trying to figure out the FakeMediaSourceEngine failure, which the build bot seems not to have. I think we could call the nondeterministic behavior a compiler bug, as well. |
Looks like you can work around FakeMediaSourceEngine by adding explicit parameter and return annotations to bufferedAheadOf. Again, something you shouldn't have to do. Another |
One more request: It looks like with the new compiler and these two changes, we now pass 'analyzerChecks'. Would you please remove |
I still have been unable to reproduce any failures. I tried 32 and 64 bit linux kernels with openjdk 7, 8 and 9. Can you share which jdk you are seeing the failures on? Something strange is going on because my linux desktop (as well as a digital ocean droplet I just spun up) compile without errors. The shaka-player-bot host compiles with one error and your machine compiles with two errors. |
After getting much debugging help from @dbhoot I think the source of this issue might be that the build scripts use os.walk() to determine the order in which input files are passed to the compiler. As the order of files returned from os.walk() is dependent on the directory entries in the file system this leads to unpredictable results across machines. I'll look into turning on For example, this ordering of files results in an error:
The error is:
However, this ordering of files results in no error:
|
Wow, that's crazy. How about we sort the files in python before passing them to the compiler? |
Sorting the files list in build_raw() fixed all the compiler errors for me. |
That is awesome that a fix is so simple. However, isn't this a sign of a larger problem? As build/all.py is not passing any What are your thoughts? |
I was honestly not familiar with We should fix this through |
Good idea about |
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
CLAs look good, thanks! |
As the testing code does not use goog.provide/goog.require the compiler is having problems sorting those files correctly. Can we merge this PR with the I'm looking into a more complete fix for the dependency_mode. Either way, I think the command lines we build shouldn't depend on ordering returned by os.walk() so sorting the files in python is a good idea even after we use dependency_mode. |
I double-checked, and as it should be, everything in lib/ has a goog.provide. The testing code doesn't need dependency tracking, as we don't really compile it. We just check it for type errors so we can catch more mistakes in our test code before they show up as test failures. We can definitely also sort the files in python, though. |
I moved the discussion of PTAL |
The tests are not compiled. We run the compiler over them with The test runner serves up all library code, uncompiled, followed by all test code. It uses closure to bootstrap the library through shaka-player.uncompiled.js. The Karma config controls everything. See the "files" array in karma.conf.js. |
True. But even |
Okay, let's discuss dependency mode in #465 as you suggested. One more pass through the build bot, and then we'll merge this. |
Thanks for all your help! I really appreciate it. |
No problem. Thank you for contributing! |
Testing in progress... |
All tests passed! |
Note, this PR depends upon #420.