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

Use workspaces #3717

Merged
merged 16 commits into from
Jun 17, 2024
Merged

Use workspaces #3717

merged 16 commits into from
Jun 17, 2024

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Jun 14, 2024

Testing out the new pub workspaces feature, it seems to work!

General stuff I ran into:

  • We had test fixture packages with general names that I had to rename, because they were listed as real dev deps in some packages and there can only be one resolution per name. Fixed by just renaming them.
  • Initially I got confusing errors about dependency overrides that I didn't understand how to resolve (we previously had dependency overrides to path dependencies in each pubspec).
    • The issue was you aren't allowed to override any package that is a part of the mono repo.
      • My first try to fix it was to move the overrides into the workspace pubspec, but I got the same error.
      • It wasn't obvious to me that I just needed to remove them entirely, they are automatically all path dependencies.
  • It looks like mono_repo will need some work to support workspaces Actually I think it is OK to run dart pub get from any dir, so it might be OK

In general, it really wasn't difficult 👍

cc @jonasfj @kevmoo @natebosch @sigurdm

@jakemac53
Copy link
Contributor Author

I am working on several more fixes related to how workspaces are handled in build_runner as well here.

@natebosch
Copy link
Member

What local workflows do we expect to be impacted? Anything outside of changes to the behavior of pub get?

Can we split this up a bit into separate PRs so that the code changes aren't mixed in with so much other noise?

@sigurdm
Copy link
Contributor

sigurdm commented Jun 17, 2024

It wasn't obvious to me that I just needed to remove them entirely, they are automatically all path dependencies.

Maybe we can improve the help message... It currently just says:

Cannot override workspace packages.
Package `$override` at `${overriddenWorkspacePackage.presentationDir}` is overridden in `${package.pubspecPath}`.

Maybe just suggesting to remove the override would be enough?

@jakemac53
Copy link
Contributor Author

jakemac53 commented Jun 17, 2024

Maybe just suggesting to remove the override would be enough?

Yeah, I think the key thing I was missing is that there is effectively an implicit (path) override already there for all workspace packages. It made sense once I understood that, it just wasn't clear to me that was there at first. My thought when seeing this message was "but I need them to be path dependencies!".

@jakemac53
Copy link
Contributor Author

@natebosch note that I had to remove watching of the .dart_tool/package_config.json file... in the workspace case it is not readable, because it lives outside of any package.

For now I just logged a warning that you would have to manually restart if you run a pub get/upgrade, because the package graph might have changed (generally this matters when new deps are added, or old ones removed).

@jakemac53 jakemac53 requested a review from natebosch June 17, 2024 18:25
@jakemac53 jakemac53 merged commit b3d3ef1 into master Jun 17, 2024
70 checks passed
@jakemac53 jakemac53 deleted the use-workspaces branch June 17, 2024 18:41
@jakemac53 jakemac53 changed the title [WIP] Use workspaces Use workspaces Jun 17, 2024
@jonasfj
Copy link
Member

jonasfj commented Jun 18, 2024

I had to remove watching of the .dart_tool/package_config.json file... in the workspace case it is not readable, because it lives outside of any package.

Inside .dart_tool/ there is a file referencing the package_config.json, so finding it should be easy.

nvm, that reference file is in .dart_tool/pub/, consider it private, don't use it 🤣

Just walk up the parent directories until you find .dart_tool/package_config.json -- that's what all other tools do.

@jakemac53
Copy link
Contributor Author

The problem isn't finding it, the problem is we have a file system abstraction which does not allow file access outside of the current package, or one of its transitive deps. Everything goes through an AssetId which is a package + path.

When running on a sub-package, the workspace packages is not a transitive dep, and so file cannot be referenced.

We also don't allow access to anything outside of lib/ from dependencies which further complicates the issue.

@sigurdm
Copy link
Contributor

sigurdm commented Jun 20, 2024

Everything goes through an AssetId which is a package + path.

Could you reach the package_config through this abstraction before? That is also outside lib/ of any package.

Maybe we need to make a special AssetId for the package-config? Or provide access to it in some other way.

@jakemac53
Copy link
Contributor Author

Could you reach the package_config through this abstraction before? That is also outside lib/ of any package.

In the root package, you are allowed to read anything, so yes it was readable. It is only for dependencies that we block anything outside of lib by default, because we only want to expose the public files.

Maybe we need to make a special AssetId for the package-config? Or provide access to it in some other way.

It breaks the model quite a lot, and I don't have the time to try and come up with a fix. It would have to a one-off for sure.

Ultimately, this just means for people using workspaces, they will have to manually shut down and restart build_runner after running pub get. I think it is probably acceptable.

@sigurdm
Copy link
Contributor

sigurdm commented Jun 21, 2024

Ultimately, this just means for people using workspaces, they will have to manually shut down and restart build_runner after running pub get. I think it is probably acceptable.

I think we can launch like that. Opening an issue for fixing this later though.

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.

4 participants