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

Simpler in memory filesystem #3823

Merged

Conversation

davidmorgan
Copy link
Contributor

@davidmorgan davidmorgan commented Feb 4, 2025

For #3811.

Switch from using the analyzer's in memory filesystem implementation, which does a lot of things that are not needed, to a local implementation that does only what is needed.

Improvements:

  • more efficient mutations to state, for example "write and check if changed" in one operation
  • tests can take advantage of checking the state (although this is not used yet)
  • reveals that currently all source passed to the analyzer does an unnecessary utf8 encode+decode and additional md5sum, this can be fixed (which I will follow up on)

Copy link

github-actions bot commented Feb 4, 2025

Package publishing

Package Version Status Publish tag (post-merge)
package:build 2.4.2 already published at pub.dev
package:build_config 1.1.2 already published at pub.dev
package:build_daemon 4.0.3 already published at pub.dev
package:build_modules 5.0.11 already published at pub.dev
package:build_resolvers 2.4.4-wip WIP (no publish necessary)
package:build_runner 2.4.15-wip WIP (no publish necessary)
package:build_runner_core 8.0.1-dev ready to publish build_runner_core-v8.0.1-dev
package:build_test 2.2.4-wip WIP (no publish necessary)
package:build_web_compilers 4.1.1 already published at pub.dev
package:scratch_space 1.0.3-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

Copy link

github-actions bot commented Feb 4, 2025

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

@davidmorgan davidmorgan force-pushed the simpler-in-memory-filesystem branch 4 times, most recently from a8fb66b to d103d23 Compare February 5, 2025 09:56
…Filesystem.

Implement the analyzer interfaces rather than using it's in-memory filesystem, which provides a lot of unused functionality.

Simplify `AnalysisDriverModel` and `BuildAssetUriResolver`.

Add unit test.
@davidmorgan davidmorgan force-pushed the simpler-in-memory-filesystem branch from d103d23 to f3c86e3 Compare February 5, 2025 09:59
@davidmorgan davidmorgan marked this pull request as ready for review February 5, 2025 10:00
@davidmorgan davidmorgan requested review from jensjoha and jakemac53 and removed request for jakemac53 and jensjoha February 5, 2025 10:01
@davidmorgan
Copy link
Contributor Author

CI found some test failures, please hold off on reviewing while I take a look :)

@davidmorgan
Copy link
Contributor Author

One of the "unused" methods was used ;) now implemented, PTAL.

///
/// Returns null if the Uri cannot be parsed.
static AssetId? parseAsset(Uri uri) {
if (const ['dart', 'dart-ext'].any(uri.isScheme)) return null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we write this similar to the line below instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually this line is not needed at all, it will anyway fall through and return null. (This was copied existing code but with the consts inlined).

Removed. Thanks.

Copy link
Contributor Author

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

///
/// Returns null if the Uri cannot be parsed.
static AssetId? parseAsset(Uri uri) {
if (const ['dart', 'dart-ext'].any(uri.isScheme)) return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually this line is not needed at all, it will anyway fall through and return null. (This was copied existing code but with the consts inlined).

Removed. Thanks.

@davidmorgan davidmorgan merged commit 7995cdd into dart-lang:master Feb 6, 2025
75 checks passed
@davidmorgan davidmorgan deleted the simpler-in-memory-filesystem branch February 6, 2025 08:55
///
/// Tracks modified paths, which should be passed to
/// `AnalysisDriver.changeFile` to update the analyzer state.
class AnalysisDriverFilesystem implements UriResolver, ResourceProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. I did not expect that clients will start implementing ResourceProvider.
This will potentially complicate making any change to this interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants