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

Demo of using a root .dart_tool/package_config.json instead of pubspec.yaml #51782

Closed
wants to merge 3 commits into from

Conversation

matanlurey
Copy link
Contributor

In this PR, I:

  • deleted tools/engine_tool/pubspec.yaml (and artifacts like locks and package_config)
  • added a root .dart_tool/package_config.json with all dependencies
  • ignored directories that have intentionally malformed Dart packages

I am not sure we'd want to land this as-is, it has the following rough spots:

  • Editing a JSON file that typically is never manually written (it's not that hard though)
  • You have to (manually) remember to delete orphaned .dart_tool/package_config.json files, or they are used instead of the root. I suppose we could automate this with some sort of git hook, but it's not done today.

Feedback welcome.

/cc @jacob314 @keertip mono repo stars.

# vended directories or code that is copied by tooling.
- prebuilts/
- third_party/
# Directories that have Dart code, but the Dart code isn't resolvable by the
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we already think we're analyzing some or all of this Dart code: https://github.com/flutter/engine/blob/main/ci/analyze.sh#L58 and below. Why are we not already hitting problems with this?

Copy link
Member

Choose a reason for hiding this comment

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

I think we'd probably want some documentation to explain how to add/remove dependencies.

@@ -0,0 +1,143 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is there are a tool or package other than pub that knows how to generate a package_config.json file with the right schema?

Copy link
Contributor

@jakemac53 jakemac53 Mar 29, 2024

Choose a reason for hiding this comment

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

The package_config package has a toJson method on PackageConfig. I am not sure what the API for creating PackageConfig objects is like, but you could use that. Ultimately it would just be more complicated I think than maintaining this file.

The Dart SDK has a bespoke tool for it also which does more sort of auto-creation based on looking in specific directories for packages, which might be an option here.

@jacob314
Copy link
Contributor

jacob314 commented Apr 1, 2024

Fyi @pq. The engine has multiple analysis_options.yaml files so this could be a good test for the changes you just enabled.

@johnmccutchan
Copy link
Contributor

@matanlurey Can we land this?

@matanlurey
Copy link
Contributor Author

@matanlurey Can we land this?

We could certainly talk about it. My personal impression was that:

  • Editing a JSON file that typically is never manually written

Is about as bad as the current workflow, so not a blocker.

  • You have to (manually) remember to delete orphaned .dart_tool/package_config.json files

This seems like a blocker. We could try to make et smart about this somehow, but in the current state (i.e. without additional tooling) makes it very easy to get your local repository in a bad state, and due to .gitignore's of the configuration files across the repo, you'd need some sort of cleanup state - and it couldn't just be 1-time, lots of tools (like VSCode) might generate new files over time :-/

The system fix is that pub is going to support something called resolution: workspace that has the semantics we want. I'm not sure of the update or when that is intended to land however.

@matanlurey
Copy link
Contributor Author

Canonical issue: flutter/flutter#147883

# .dart_tool but allow the .dart_tool/package_config.json file.
.dart_tool/**
!.dart_tool/package_config.json
# But ignore the .dart_tool directory in any subdirectories.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/But/and

@johnmccutchan johnmccutchan removed their request for review June 26, 2024 16:48
@auto-submit auto-submit bot closed this in #53539 Jul 18, 2024
@auto-submit auto-submit bot closed this in 8bcf638 Jul 18, 2024
itsjustkevin pushed a commit to itsjustkevin/engine that referenced this pull request Jul 19, 2024
…3539)

... and use it in `engine_tool` to prove everything is working, i.e. on CI and elsewhere.

Partial work towards flutter/flutter#147883.
Supersedes and closes flutter#51782 (which was a prototype).

See also:
- https://flutter.dev/go/pub-workspace, the design doc on the feature.
- dart-lang/pub-dev#7762, an example PR.

I'll probably end up moving the inline docs in `pubspec.yaml` to a `docs/*.md` once that's a thing.
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